Nested Finance contest - harleythedog's results

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.