Foundry DeFi Stablecoin CodeHawks Audit Contest
Cyfrin
DeFiFoundry
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Tokens with missing return values in transfers won't work as expected as collateral

kose

Summary

If users use tokens that don't have return values in transfer (or have return values in some methods but not others) protocol won't work as expected.

Vulnerability Details

Some tokens do not return a bool on ERC20 methods (for example USDT, OMG) and some tokens may return a bool for some methods but not for all of them. Let's look at what our contract is expecting during deposit and redeem:

function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
public
moreThanZero(amountCollateral)
isAllowedToken(tokenCollateralAddress)
nonReentrant
{
s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
}
...
​
function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
private
{
s_collateralDeposited[from][tokenCollateralAddress] -= amountCollateral;
emit CollateralRedeemed(from, to, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transfer(to, amountCollateral); // @audit - Return value, BNB
if (!success) {
revert DSCEngine__TransferFailed();
}
}

As we can see if transfer and transferfrom does not return true, these operations will revert.

1- Tokens with no return values such as USDT and OMG won't be available to use as collateral against protocol expectations.

2- Tokens that have return value for some functions but not for all such as BNB will create more problems:
BNB has a return value for transferFrom but not for transfer. So it is possible for users to depositCollateral using BNB, but it is impossible to redeemCollateral because it will always revert. Hence users deposited collateral will be stuck in contract.

Impact

Some tokens won't be available to use as collateral, but some tokens will allow depositing while revert in redeeming. Hence users funds will be locked in protocol forever. Since there is no way to recover these funds and protocol is expected to work with any token that have priceFeed in ChainLink, I consider this as high.

Tools Used

Manual Review

Recommendations

Either don't approve these tokens as collateral, or remove return value check for transfers.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.