ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements or use safe wrapper functions implementing return value/data checks to handle these failures. For reference, see similar Medium-severity finding from Consensys Diligence Audit of Aave Protocol V2: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom
While the contract uses OpenZeppelin’s SafeERC20 functions in most places, it misses using safeTransfer() in two cases. The use of transfer() for basketAsERC20 token is ok because it is this protocol’s token. However, the use of transfer() for bounty token transfers is risky because bounty tokens are arbitrary tokens added by any one, including a malicious publisher.
The impact can be that for an arbitrary ERC20 token, this transfer() call may return failure but the bounty logic misses that, assumes it was successfully transferred to user.
GalloDaSballo
commented
2 years ago
Handle
0xRajeev
Vulnerability details
Impact
ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements or use safe wrapper functions implementing return value/data checks to handle these failures. For reference, see similar Medium-severity finding from Consensys Diligence Audit of Aave Protocol V2: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom
While the contract uses OpenZeppelin’s SafeERC20 functions in most places, it misses using safeTransfer() in two cases. The use of transfer() for basketAsERC20 token is ok because it is this protocol’s token. However, the use of transfer() for bounty token transfers is risky because bounty tokens are arbitrary tokens added by any one, including a malicious publisher.
The impact can be that for an arbitrary ERC20 token, this transfer() call may return failure but the bounty logic misses that, assumes it was successfully transferred to user.
Proof of Concept
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L146
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L101
Tools Used
Manual Analysis
Recommended Mitigation Steps
Use safeTransfer instead of transfer given that the bounty token is untrusted.