mstpr-brainbot - Some curve pools will not be able to remove liquidity single sided

[sherlock-admin2]

mstpr-brainbot

medium

Some curve pools will not be able to remove liquidity single sided

Summary

Some curve pools uses the remove_liquidity_one_coin function with uint256 but the notional code assumes it is always int128 which if the pool to be used is using uint256 then the removing single sided liquidity is impossible.

Vulnerability Detail

First this is the interface that the Notional uses when removing single sided liquidity from curve:

interface ICurve2TokenPool is ICurvePool {
    function remove_liquidity(uint256 amount, uint256[2] calldata _min_amounts) external returns (uint256[2] memory);
    function remove_liquidity_one_coin(uint256 _token_amount, int128 i, uint256 _min_amount) external returns (uint256);
}

so it uses uint256, int128 and uint256 as inputs. Let's see some of the pools that notional can potentially use. One can be the rETH-ETH pool for ETH leveraged vault. This is how the remove_liquidity_one_coin function is built:

@external
@nonreentrant('lock')
def remove_liquidity_one_coin(token_amount: uint256, i: uint256, min_amount: uint256,
                              use_eth: bool = False, receiver: address = msg.sender) -> uint256:

as we can see it uses uint256, uint256 , uint256. In Notionals interface above it uses int128 for the I variable but the actual pool uses uint256. This will fail because casting an interface input from int128 to uint256 is not supported in solidity.

Impact

Pool can be deployed without a problem. However, in production the removing single sided liquidity will not work. Also, Notional team states that every "potential" curve pool can be used and the above example is rETH-ETH which is a very good candidate for ETH strategy. Thus, I'll label this as medium.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol#L78-L84

Tool used

Manual Review

Recommendation

Make a flag that tells the contract that the pool uses remove_liquidity_one_coin with uint256 inputs.

[jeffywu]

Is this actually true? Unless the i value is negative, it will be abi encoded in the same way that a uint256 value is encoded and it should be properly passed to the curve contract.

[nevillehuang]

Hi @jeffywu is there a foundry test proving that it will be properly passed to the curve contract?

[jeffywu]

Yes there is a test against Curve pools here: https://github.com/notional-finance/leveraged-vaults/blob/fix-review/tests/SingleSidedLP/pools/FRAX_USDC_e.t.sol

It runs a number of vault exits that will execute the code above: https://github.com/notional-finance/leveraged-vaults/blob/fix-review/tests/BaseAcceptanceTest.sol#L232

https://github.com/notional-finance/leveraged-vaults/blob/fix-review/contracts/vaults/Curve2TokenConvexVault.sol#L80C1-L85C17

Taking a closer look at this, the actual issue is that CurveV2 pools use a different method signature from what our interface is defining and therefore we will actually get a revert when trying to call the remove_liquidity_one_coin function. The casting from uint256 to int256 is not relevant.

I would say that this a duplicate of this issue: https://github.com/sherlock-audit/2023-10-notional-judging/issues/86

They are related in the sense that we are not using the proper method signature for CurveV2 pools and therefore we will not be able to exit liquidity in those cases. In issue #86 we will not actually lose the ETH, the contract will revert because the method signatures will not match (the int256 and uint256 will encode to different signatures).

[nevillehuang]

Since the root cause identified in this issue is not an actual vulnerability, going to close this issue as invalid. Just to double check, the casting from int128 to uint256 for i is not an issue correct?

[jeffywu]

No, i in all of the cases refers to the index of the coins which is never that large of a value. In our case we cast from a uint8 to either int128 or uint256 and therefore there would never be any sort of overflow / underflow issue.