Users will lose their staked rewards when a curve pool is killed

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x3a6c53b83890a66f0d000cf589585339ccbd74c3a15fd8630902b77ba562fa06 Severity: medium

Description:

Impact

CvgCvxStakingPositionService deposit() and withdraw() are permanently (or temporarily if unpaused) inaccessible. Possible long-term (or short-term if the pool is unpaused) freezing of user funds. Note that the impact is present when a curve pool used is killed.

Description

Whenever a Curve pool is paused or killed the pool's is_killed variable will be set to true. The problem is that every curve pool .exchange() call will revert in this case making essential functionality of the contracts unusable.

Tricrypto (and numerous other) curve pool's exchange() function:

@payable
@external
@nonreentrant('lock')
def exchange(i: uint256, j: uint256, dx: uint256, min_dy: uint256, use_eth: bool = False):
    assert not self.is_killed # dev: the pool is killed
    assert i != j # dev: coin index out of range
    assert i < N_COINS # dev: coin index out of range
    assert j < N_COINS # dev: coin index out of range
    assert dx > 0  # dev: do not exchange 0 coins

The most problematic scenario that I can currently see with this vulnerability is that users will be unable to withdraw therefore lose their staked reward since the staking position service's withdraw() will revert.

Impacted contracts - functions every curvePool.exchange() call:

• CvxRewardDistributor - _withdrawRewards() -> curve swap
• CvgCvxStakingPositionService.sol - withdraw() -> curve CVX1 swaps (2)
• CvgCvxStakingPositionService.sol - deposit() - _convertCvxToCvgCvx()-> curve convert swap
• CvxAssetStakingService.sol - _deposit() -> _mintOrSwapToCvxAsset() -> curve swap

Recommendation

Consider to develop safeguards against the scenario where a curve pool is killed. Consider to have an alternative way to withdraw staked cvgCVX. Furthermore consider to use a bot that front-runs kill_me() txs of the used curve pools to save assets in a case a curve pool for some reason is permanently killed. I can provide further recommendations if needed.

[PlamenTSV]

I believe this is a centralization risk - OOS, since the curve pools for CVG assets will be managed by Convergance. Even so, I will leave this to the sponsor, since pausing a pool technically leads to your scenario. Keep in mind that a Curve pool cannot be killed after the first 30 days as well.

[0xfuje]

I can totally see your reasoning however I still think this is dangerous design that can unnecessarily block functionality and freeze user funds.

[0xR3vert]

If a pool is killed for x reason (which is very rare, and honestly won't happen), we adapt with upgradeable contracts. What's more, if the pool is in this situation, deposits and withdrawals will still be possible, just certain ways of depositing/withdrawing will not be accessible. So even if this happens one day users won't be blocked, and as PlamenTSV said the pool call only be killed in the first 30 days. In conclusion this issue is considered invalid.