ERC20 implementations may revert on zero approval

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/Base.sol#L59 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L152-L153 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L202-L203

Vulnerability details

Summary

Some ERC20 implementations revert when approve() is called with a zero amount, causing a denial of service when token allowances are reset.

Impact

Token allowances in the LAMM implementation follow a pattern in which approvals are setup for the required amount before executing the operation, and then reset back to zero after executing the operation.

The same behavior is present in Base.swap(), LiquidityPosition.mint() and LiquidityPosition.increaseLiquidity(). Taking the latter as an example, we can see the implementation calls safeApprove() with the required amount, then executes the call to increaseLiquidity() in the Uniswap NPM contract, and finally it resets the allowances back to zero by executing another call to safeApprove(), to ensure any unused allowance is cleared.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L178-L204

178:     function increaseLiquidity(
179:         address token0,
180:         address token1,
181:         uint256 tokenId,
182:         uint256 amount0,
183:         uint256 amount1
184:     ) internal returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
185:         // approve spending for uniswap's position manager
186:         TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, amount0);
187:         TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, amount1);
188: 
189:         // increase liquidity via position manager
190:         (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
191:             INonfungiblePositionManager.IncreaseLiquidityParams({
192:                 tokenId: tokenId,
193:                 amount0Desired: amount0,
194:                 amount1Desired: amount1,
195:                 amount0Min: 0,
196:                 amount1Min: 0,
197:                 deadline: block.timestamp
198:             })
199:         );
200: 
201:         // reset approval
202:         TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, 0);
203:         TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, 0);
204:     }

Some ERC20 implementations revert on approvals of zero value. One major example is the BNB token, that throws when called with 0 as the amount argument.

This will cause a denial of service in any of the mentioned functions, preventing the protocol from being used under the presence of such tokens.

Recommendation

There is no clean solution for this issue, a potential workaround could be to set the allowance to 1 wei:

1. First, check the current allowance. If it is zero already, skip any modification (this will also save gas).
2. Execute a non-reverting call to approve() using zero.
3. If the previous call fails, then execute an approval for 1 wei.

Assessed type

ERC20

[0xleastwood]

Non-standard token type support is more of a QA issue to me.

[c4-judge]

0xleastwood marked the issue as primary issue

[wukong-particle]

I agree with the judge @0xleastwood. But good to learn this perk for BNB token.

Re check allowance, aren't the previous approvals always set the approved value to be higher?

[c4-sponsor]

wukong-particle (sponsor) acknowledged

[0xleastwood]

I agree with the judge @0xleastwood. But good to learn this perk for BNB token.

Re check allowance, aren't the previous approvals always set the approved value to be higher?

Not super concerned about this being an issue. It's not possible for LPs to mint LP tokens in the first place and therefore positions cannot be opened. This is a compatibility issue and no funds are at risk. Downgrading to QA.

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)