SAFEAPPROVE() MAY REVERT FUNCTIONS

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L102
https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L146

Vulnerability details

SAFEAPPROVE() MAY REVERT FUNCTIONS

In _depositToYieldPool(), in the vaults contracts, safeApprove() from the OpenZeppelin’s SafeERC20 library is used to give allowance to the Lending Pool address to transfer the amount to be deposited.

However, the safeApprove function prevents changing an allowance between non-zero values to mitigate a possible front-running attack. It reverts if that is the case. Instead, the safeIncreaseAllowance and safeDecreaseAllowance functions should be used. Comment from the OZ library for this function:

safeApprove should only be called when setting an initial allowance, or when resetting it to zero. To increase and decrease it, use safeIncreaseAllowance and safeDecreaseAllowance

If the existing allowance is non-zero (ie if a user deposits a second time or more), then safeApprove() will revert causing the user’s token deposits to fail, leading to denial-of-service.

Impact

Medium

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use safeIncreaseAllowance() instead of safeApprove().

[HickupHH3]

I agree with the finding, but previous contests have marked it as low-risk (QA).
See:
code-423n4/2021-11-malt-findings#41
code-423n4/2022-01-sherlock-findings#269

Hence, downgrading to QA.

[HickupHH3]

Grouping this with the warden’s QA report: #111