token.approve() doesn’t check return value

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

Multiple files within the contracts/basket/contracts/singleJoinExit/ directory call token.approve() for an ERC20 token, but these calls do not verify whether the approve() call failed. Some ERC20 tokens do not revert if an approval fails, and because the return value is not checked, the contract would not be aware of this failure, potentially causing malfunctions in later operations. Using a function from SafeERC20 that checks the return value would mitigate this edge case.

Proof of Concept

token.approve() is found in several locations:

• singleJoinExit/SingleTokenJoin.sol line 43
• singleJoinExit/SingleNativeTokenExitV2.sol line 55
• singleJoinExit/SingleNativeTokenExit.sol line 44
• singleJoinExit/SingleTokenJoinV2.sol line 53

Tools Used

Manual analysis

Recommended Mitigation Steps

While the OpenZeppelin SafeERC20 safeApprove() function could be used to revert on approve failures unlike the standard approve(), the safeApprove() function is deprecated and instead OpenZeppelin recommends either safeIncreaseAllowance() or safeDecreaseAllowance(). Because uint256(-1) should be an increase, replace each instance of token.approve(spender, uint256(-1)) with token.safeIncreaseAllowance(spender, uint256(-1)).

[loki-sama]

In our case safeApprove() would be better as we don't care about front running the approval and safeIncreaseAllowance() would fail when allowance is bigger then 0.