ExchangeHelpers.sol setMaxAllowance does not mitigate race condition as described #70
Labels
1 (Low Risk)
Assets are not at risk. State handling, function incorrect as to spec, issues with comments
bug
Something isn't working
duplicate
This issue or pull request already exists
Handle
harleythedog
Vulnerability details
Impact
In ExchangeHelpers.sol, the setMaxAllowance function has the following code:
// Approve 0 first for tokens mitigating the race condition
_token.safeApprove(_spender, 0);
_token.safeApprove(_spender, type(uint256).max);
However, this sequence of approvals does not mitigate the race condition referenced by the comment. Since we are setting the approval amount to 0 and then resetting it to the non-zero value in the same transaction, a front-running attack could still use both approval amounts. The use of safeApprove in SafeERC20 is discouraged when safeIncreaseAllowance/safeDecreaseAllowance are possible (see the comment in the openzeppelin implementation here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#:~:text=%7D-,/**,*/,-function%20safeApprove().
I am putting this as a medium severity instead of a high severity, since the function is setting max allowance to the _spender anyways. However, this is still an issue, since _spender may end up using more of the allowance than you think, before you give them max allowance.
Proof of Concept
Referenced code is here: https://github.com/code-423n4/2021-11-nested/blob/main/contracts/libraries/ExchangeHelpers.sol#:~:text=the%20race%20condition-,_token,-.safeApprove(_spender%2C%200
Since the code sets the allowance to 0 and then sets it to a non-zero value in the same transaction (instead of waiting for the zero allowance setting transaction to be mined before setting the non-zero value), the race condition still persists, which is obviously not intended based on the comment in the code.
Tools Used
Manual inspection.
Recommended Mitigation Steps
Instead use safeIncreaseAllowance to set the allowance of the user to the value desired.
The text was updated successfully, but these errors were encountered: