0x52 - CollateralManager#commitCollateral overwrites collateralInfo._amount if called with an existing collateral

Question

0x52 - CollateralManager#commitCollateral overwrites collateralInfo._amount if called with an existing collateral

Opened this issue a year ago · 10 comments

0x52

high

CollateralManager#commitCollateral overwrites collateralInfo._amount if called with an existing collateral

Summary

When duplicate collateral is committed, the collateral amount is overwritten with the new value. This allows borrowers to front-run bid acceptance to change their collateral and steal from lenders.

Vulnerability Detail

CollateralManager.sol#L426-L442

function _commitCollateral(
    uint256 _bidId,
    Collateral memory _collateralInfo
) internal virtual {
    CollateralInfo storage collateral = _bidCollaterals[_bidId];
    collateral.collateralAddresses.add(_collateralInfo._collateralAddress);
    collateral.collateralInfo[
        _collateralInfo._collateralAddress
    ] = _collateralInfo; <- @audit-issue collateral info overwritten
    emit CollateralCommitted(
        _bidId,
        _collateralInfo._collateralType,
        _collateralInfo._collateralAddress,
        _collateralInfo._amount,
        _collateralInfo._tokenId
    );
}

When a duplicate collateral is committed it overwrites the collateralInfo for that token, which is used to determine how much collateral to escrow from the borrower.

TellerV2.sol#L470-L484

function lenderAcceptBid(uint256 _bidId)
    external
    override
    pendingBid(_bidId, "lenderAcceptBid")
    whenNotPaused
    returns (
        uint256 amountToProtocol,
        uint256 amountToMarketplace,
        uint256 amountToBorrower
    )
{
    // Retrieve bid
    Bid storage bid = bids[_bidId];

    address sender = _msgSenderForMarket(bid.marketplaceId);

TellerV2#lenderAcceptBid only allows the lender input the bidId of the bid they wish to accept, not allowing them to specify the expected collateral. This allows lenders to be honeypot and front-run causing massive loss of funds:

Malicious user creates and commits a bid to take a loan of 10e18 ETH against 100,000e6 USDC with 15% APR
Lender sees this and calls TellerV2#lenderAcceptBid
Malicious user front-runs transaction with commitCollateral call setting USDC to 1
Bid is filled sending malicious user 10e18 ETH and escrowing 1 USDC
Attacker doesn't repay loan and has stolen 10e18 ETH for the price of 1 USDC

Impact

Bid acceptance can be front-run to cause massive losses to lenders

Code Snippet

TellerV2.sol#L470-L558

Tool used

Manual Review

Recommendation

Allow lender to specify collateral info and check that it matches the committed addresses and amounts

IAm0x52 commented a year ago

PR: #101

Answer 1 · 2023-05-13T17:57:37.000Z

The lenderAcceptBid function will revert if it tries to deposit duplicated collateral because the CollateralEscrowV1.depositAsset function has a requirement to avoid asset overwriting (https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L66-L70). Therefore, if the borrower front-runs and changes the collateral amount, the lenderAcceptBid function will revert, causing the lender to lose gas only.

Answer 2 · 2023-05-15T15:41:01.000Z

as trumpero says, this issue has been known and so we put in reversions to prevent this from happening. However we do want to eventually fix this in a better way so that you will be able to use two of the same ERC721 as collateral for one loan at some point. Ex. using two bored apes as collateral for one loan. This is not possible due to our patch for this.

Answer 3 · 2023-05-20T12:19:19.000Z

Escalate for 10 USDC

The validness and the High severity of this issue is proved with a coded POC in #250.

The lenderAcceptBid function will revert if it tries to deposit duplicated collateral because the CollateralEscrowV1.depositAsset function has a requirement to avoid asset overwriting (https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L66-L70). Therefore, if the borrower front-runs and changes the collateral amount, the lenderAcceptBid function will revert, causing the lender to lose gas only.

The comment addresses the deposit action, but the attack is actually performed before that, during commitCollateral. Commited collateral can actually be updated, which is the root cause of this issue.

A malicious borrower submits a bid (collateral is commited but not deposited).
A victim lender decides to accept the bid and sends the tx to the mempool.
The malicious borrower sees the tx in the mempool and frontruns it by updating the commited collateral to a minimum.
The accept bid tx from the victim lender succeeds and the borrower collateral is finally deposited (but just a minimum).

Take note that funds are deposited once via deployAndDeposit, which is called during the lenderAcceptBid function.

The impact is High because assets are lent with a minimum collateral provided (of as low as 1 wei). This can be consider stealing users funds.

A coded proof is provided in #250 , which I also suggest to be used for the final report, as it demonstrates the issue with a POC and provides coded recommendations to mitigate the issue.

Answer 4 · 2023-05-20T12:19:21.000Z

Escalate for 10 USDC

The validness and the High severity of this issue is proved with a coded POC in #250.

The lenderAcceptBid function will revert if it tries to deposit duplicated collateral because the CollateralEscrowV1.depositAsset function has a requirement to avoid asset overwriting (https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L66-L70). Therefore, if the borrower front-runs and changes the collateral amount, the lenderAcceptBid function will revert, causing the lender to lose gas only.

The comment addresses the deposit action, but the attack is actually performed before that, during commitCollateral. Commited collateral can actually be updated, which is the root cause of this issue.

A malicious borrower submits a bid (collateral is commited but not deposited).

A victim lender decides to accept the bid and sends the tx to the mempool.

The malicious borrower sees the tx in the mempool and frontruns it by updating the commited collateral to a minimum.

The accept bid tx from the victim lender succeeds and the borrower collateral is finally deposited (but just a minimum).

Take note that funds are deposited once via deployAndDeposit, which is called during the lenderAcceptBid function.

The impact is High because assets are lent with a minimum collateral provided (of as low as 1 wei). This can be consider stealing users funds.

A coded proof is provided in #250 , which I also suggest to be used for the final report, as it demonstrates the issue with a POC and provides coded recommendations to mitigate the issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Answer 5 · 2023-05-21T18:47:16.000Z

The CollateralManager will never attempt to deposit duplicate collateral, the code for committing collateral is the following:

    function _commitCollateral(
        uint256 _bidId,
        Collateral memory _collateralInfo
    ) internal virtual {
        CollateralInfo storage collateral = _bidCollaterals[_bidId];
        collateral.collateralAddresses.add(_collateralInfo._collateralAddress);
        collateral.collateralInfo[
            _collateralInfo._collateralAddress
        ] = _collateralInfo;
        ...
    }

The set collateral.collateralAddresses will not contain the address of the collateral twice since OZ's EnumerableSetUpgradeable does not add an element to a set if it is already present (it's a set, not a list). The previous value for collateral.collateralInfo[_collateralInfo._collateralAddress] will be replaced with no additional side-effect.

Hence the check for duplicate collateral in CollateralEscrowV1 is never triggered:

    function depositAsset(
        CollateralType _collateralType,
        address _collateralAddress,
        uint256 _amount,
        uint256 _tokenId
    ) external payable virtual onlyOwner {
        require(_amount > 0, "Deposit amount cannot be zero");
        _depositCollateral(
            _collateralType,
            _collateralAddress,
            _amount,
            _tokenId
        );
        Collateral storage collateral = collateralBalances[_collateralAddress];

        //Avoids asset overwriting.  Can get rid of this restriction by restructuring collateral balances storage so it isnt a mapping based on address.
        require(
            collateral._amount == 0,  // @audit this check is never triggered
            "Unable to deposit multiple collateral asset instances of the same contract address."
        );

        collateral._collateralType = _collateralType;
        collateral._amount = _amount;
        collateral._tokenId = _tokenId;
        emit CollateralDeposited(_collateralAddress, _amount);
    }

See OZ's docs

Answer 6 · 2023-06-08T18:00:48.000Z

Your comment about '@Audit this check is never triggered' is disproven by this test which had already been in the repo.

function test_depositAsset_ERC721_double_collateral_overwrite_prevention()
    public
{
    CollateralEscrowV1_Override escrow = CollateralEscrowV1_Override(
        address(borrower.getEscrow())
    );

    uint256 tokenIdA = erc721Mock.mint(address(borrower));
    uint256 tokenIdB = erc721Mock.mint(address(borrower));

    borrower.approveERC721(address(erc721Mock), tokenIdA);
    borrower.approveERC721(address(erc721Mock), tokenIdB);

    vm.prank(address(borrower));
    escrow.depositAsset(
        CollateralType.ERC721,
        address(erc721Mock),
        1,
        tokenIdA
    );

    uint256 storedBalance = borrower.getBalance(address(erc721Mock));

    assertEq(storedBalance, 1, "Escrow deposit unsuccessful");

    vm.expectRevert(
        "Unable to deposit multiple collateral asset instances of the same contract address."
    );
    vm.prank(address(borrower));
    escrow.depositAsset(
        CollateralType.ERC721,
        address(erc721Mock),
        1,
        tokenIdB
    );
}

Answer 7 · 2023-06-09T14:19:56.000Z

Escalation accepted

Valid high
After further review and discussions, this is a valid high issue and the POC in #250 proves the same.

Answer 8 · 2023-06-09T14:19:59.000Z

Escalation accepted

Valid high
After further review and discussions, this is a valid high issue and the POC in #250 proves the same.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Answer 9 · 2023-06-14T16:25:12.000Z

Fix looks good. Commit will revert if collateral is already present