yixxas - Adversary can prevent withdrawal of assets if token used has multiple addresses

[sherlock-admin]

yixxas

high

Adversary can prevent withdrawal of assets if token used has multiple addresses

Summary

Some tokens have multiple addresses. If such tokens are used as collatearl, an adversary can cause unknown users from having their collateral stuck in the contract permanently.

Vulnerability Detail

commitCollateral() has no access control. It checks the balance of borrower to ensure that they have enough balance. If a check is successful, the address is added to the collateralAddresses enumerableSet via collateral.collateralAddresses.add(_collateralInfo._collateralAddress).

The issue here is that, for a token with multiple addresses, an adversary can call this function to add the same token of its different addresses to collateral.collateralAddresses. This call will succeed as commitCollateral() only checks balances of the token, and both addresses will map to the same balances of the token.

When withdrawing, it loops through all collateralAddresses of thebidId. It then withdraws the token based on collateralInfo._amount.

For example, if a user has balanceOf(tokenId) = 500, and collateralInfo._amount = 500, after the first withdrawal, balanceOf == 0, but it will attempt to withdraw again on this 0 balance as the 2 different addresses maps to the same token. This will revert hence preventing any withdrawal from happening permanently.

    function _withdraw(uint256 _bidId, address _receiver) internal virtual {
        for (
            uint256 i;
            i < _bidCollaterals[_bidId].collateralAddresses.length();
            i++
        ) {
            // Get collateral info
            Collateral storage collateralInfo = _bidCollaterals[_bidId]
                .collateralInfo[
                    _bidCollaterals[_bidId].collateralAddresses.at(i)
                ];
            // Withdraw collateral from escrow and send it to bid lender
            ICollateralEscrowV1(_escrows[_bidId]).withdraw(
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                _receiver
            );
            emit CollateralWithdrawn(
                _bidId,
                collateralInfo._collateralType,
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                collateralInfo._tokenId,
                _receiver
            );
        }
    }

Impact

Assets with multiple addresses can be forced trapped in contract permanently.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L431
https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L393-L419

Tool used

Manual Review

Recommendation

Consider using a whitelist to prevent such tokens from being used.

[ethereumdegen]

'Some tokens have multiple addresses' I have never heard of a token having multiple assets. This does not make sense. A token is created by the deployment of a smart contract and that smart contract address is the token address. I do not see any issue here.

[yixxas]

Escalate for 10 USDC

To respond to the sponsor @ethereumdegen, tokens with multiple addresses do exist though they are rare. A similar issue has been reported and accepted in a previous contest sherlock-audit/2023-03-taurus-judging#31

It is explicitly mentioned by the protocol that it aims to support all ERC20 tokens, hence they should be prepared to handle such tokens. Therefore, this issue should be considered a valid medium.

[sherlock-admin]

Escalate for 10 USDC

To respond to the sponsor @ethereumdegen, tokens with multiple addresses do exist though they are rare. A similar issue has been reported and accepted in a previous contest sherlock-audit/2023-03-taurus-judging#31

It is explicitly mentioned by the protocol that it aims to support all ERC20 tokens, hence they should be prepared to handle such tokens. Therefore, this issue should be considered a valid medium.

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.

[Trumpero]

Disagree with the escalation, I believe this issue is invalid. In the scenario where borrowers commit the same token with two different addresses, they are required to deposit this token twice (refer to deployAndDeposit function). Therefore, when it comes to withdrawing, no issue arises as the token amount is insufficient to withdraw twice.

[hrishibhat]

Escalation rejected

Invalid issue
As pointed out by the lead judge in order to withdraw through two different addresses, they need to deposit twice in order to withdraw twice.

[sherlock-admin]

Escalation rejected

Invalid issue
As pointed out by the lead judge in order to withdraw through two different addresses, they need to deposit twice in order to withdraw twice.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.