jennifer37 - incorrect allowance check in NapierRouter::removeLiquidityOneUnderlying()

[sherlock-admin2]

jennifer37

medium

incorrect allowance check in NapierRouter::removeLiquidityOneUnderlying()

Summary

incorrect allowance check in NapierRouter::removeLiquidityOneUnderlying()

Vulnerability Detail

In function removeLiquidityOneUnderlying(), we remove liquidity from pool to get underlying token and baseLpt in router. And then we will swap baseLpt to underlying in pool. So before we swap baseLpt to underlying, we need to check whether pool has enough allowance to transfer router's baseLpt token.

In function removeLiquidityOneUnderlying(), before router 
    function removeLiquidityOneUnderlying(
        address pool,
        uint256 index,
        uint256 liquidity,
        uint256 underlyingOutMin,
        address recipient,
        uint256 deadline
    ) external override nonReentrant checkDeadline(deadline) returns (uint256) {
        ...
        // The withdrawn base LP token is exchanged for underlying in two different ways depending on the maturity.
        // If maturity has passed, redeem else swap for underlying.
        // 1. Swapping is used when maturity hasn't passed because redeeming is disabled before maturity.
        // 2. Redeeming is preferred because it doesn't cause slippage.
        if (block.timestamp < INapierPool(pool).maturity()) {
            // Swap base LP token for underlying
            // approve max
    @==>if (IERC20(basePool).allowance(address(this), basePool) < baseLptOut) {
                IERC20(basePool).forceApprove(pool, type(uint256).max);
            }
            uint256 removed = INapierPool(pool).swapExactBaseLpTokenForUnderlying(baseLptOut, address(this));
            underlyingOut += removed;
        }

It should be

        if (block.timestamp < INapierPool(pool).maturity()) {
            // Swap base LP token for underlying
            // approve max
            // (pool, not base pool)
@==>        if (IERC20(basePool).allowance(address(this), pool) < baseLptOut) {
                IERC20(basePool).forceApprove(pool, type(uint256).max);
            }
            uint256 removed = INapierPool(pool).swapExactBaseLpTokenForUnderlying(baseLptOut, address(this));
            underlyingOut += removed;
        }

Impact

Incorrect allowance check.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/NapierRouter.sol#L744-L752

Tool used

Manual Review

Recommendation

change basePool to pool

        if (block.timestamp < INapierPool(pool).maturity()) {
            // Swap base LP token for underlying
            // approve max
   ==>    if (IERC20(basePool).allowance(address(this), pool) < baseLptOut) {
                IERC20(basePool).forceApprove(pool, type(uint256).max);
            }
            uint256 removed = INapierPool(pool).swapExactBaseLpTokenForUnderlying(baseLptOut, address(this));
            underlyingOut += removed;
        } 

[cvetanovv]

Escalate

After reviewing the report again, it was found that this change that Watson suggests gives the same result (i.e. it is not a bug). I therefore request that the report be invalidated.

[sherlock-admin4]

Escalate

After reviewing the report again, it was found that this change that Watson suggests gives the same result (i.e. it is not a bug). I therefore request that the report be invalidated.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[johnson37]

Escalate

After reviewing the report again, it was found that this change that Watson suggests gives the same result (i.e. it is not a bug). I therefore request that the report be invalidated.

Hi @cvetanovv , can you please explain a little bit why this change give the same result?

In current implementation as below, contract checks spender basePool's allowance on behalf of NapierRouter. But actually, we need to check pool's allowance on behalf of NapierRouter.

Per my understanding, basePool is different with pool.

Current Implementation:

            if (IERC20(basePool).allowance(address(this), basePool) < baseLptOut) {
                IERC20(basePool).forceApprove(pool, type(uint256).max);
            }
            uint256 removed = INapierPool(pool).swapExactBaseLpTokenForUnderlying(baseLptOut, address(this));

So the recommendation is to change basePool to pool. Because in next step, pool will transfer router's tokens.

  @--> if (IERC20(basePool).allowance(address(this), pool) < baseLptOut) {
                IERC20(basePool).forceApprove(pool, type(uint256).max);
            }
            uint256 removed = INapierPool(pool).swapExactBaseLpTokenForUnderlying(baseLptOut, address(this));

Correct me if I'm wrong.

Thanks a lot.

[cvetanovv]

Protocol have done tests and there is no difference. If you can provide a PoC that confirms the report, then your report will be valid.

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/napierfi/v1-pool/pull/154.

[Czar102]

@johnson37 if a PoC is not provided within 24h, I'm planning to accept the escalation and invalidate the issue.

[Czar102]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cvetanovv: accepted

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.