Curve2PoolAdapter & CurveTricryptoAdapter - Potential Fund Loss Due to Reversion on Curve Pool Withdrawal

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L170 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L214 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L219

Vulnerability details

Impact

The vulnerability poses a severe risk, potentially leading to the loss of funds. If the underlying Curve pool's self.is_killed becomes true, the primitiveOutputAmount() function may fail to execute, preventing the intended withdrawal operation. Consequently, funds remain locked in the Curve pool perpetually.

Proof of Concept

The root cause of the vulnerability is that the primitiveOutputAmount() function in the Curve2PoolAdapter and CurveTricryptoAdapter contracts relies on the remove_liquidity_one_coin function in the underlying Curve pool contract. If the Curve pool is killed (self.is_killed is true), calling remove_liquidity_one_coin always reverts at the assertion, leading to the failure of the withdrawal process.

Curve StableSwap3Pool.vy - Line 674:

def remove_liquidity_one_coin(_token_amount: uint256, i: int128, min_amount: uint256):
    """
    Remove _amount of liquidity all in a form of coin i
    """
    assert not self.is_killed  # dev: is killed

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to enhance the primitiveOutputAmount() functions in Curve2PoolAdapter and CurveTricryptoAdapter contracts to handle the scenario where the Curve pool is killed. The function should check for the Curve pool's isKilled status and adjust the call to remove_liquidity instead of remove_liquidity_one_coin.

On the other hand, Curve does not hold any ability to pause (via kill_me function) or emergency withdraw after a pool has been live for 30 days. Ocean should consider to ONLY support vaults on pools that have been live MORE than this period of time. (Curve DAO Emergency Withdraw)

Note: The README.md known issues section did mention the external contract risk involving the Curve contract as follows:

// File: README.md
29:* external contract risk (curve in this case)

I would argue that the killed Curve pool situation does not constitute a risk since it's their normal/expected operation. They also provide the alternative method remove_liquidity to do the withdrawal just in case, so it's the integrator's responsibility to call the appropriate endpoint when the expected situation occurs.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #54

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[hungdoo]

Hi @0xA5DF , Thanks for the judging.

I would like remark that this issue is not a duplicate of #54 which reported a finding that requires an integration with the attacker's malicious primitive. While this issue discussed a different vulnerability regarding the potential killed/paused action executed on an existing trusted Curve Pool primitive.

[c4-judge]

0xA5DF marked the issue as not a duplicate

[0xA5DF]

You're right, re-opening this @viraj124, can you take a look at this one?

[c4-judge]

0xA5DF removed the grade

[viraj124]

@0xA5DF I'd put this in the category of a known risk when interacting with external contracts so we don't think this is valid

[c4-sponsor]

viraj124 (sponsor) disputed

[0xA5DF]

I don't think this falls under the category of 'external contract risk' as users interacting directly with Curve wouldn't have this issue (they can still withdraw via remove_liquidity()).

However, regarding this part:

Consequently, funds remain locked in the Curve pool perpetually.

I think the warden is wrong about this as users can still interact directly with Curve and no funds are locked in the adapter.

Leaving this open till the end of PJQA in case the warden has any further comments.

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Closing for now