No slippage protection on rewards deposits

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/strategies/votium/VotiumStrategyCore.sol#L239 https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/strategies/votium/VotiumStrategyCore.sol#L272

Vulnerability details

Impact

Rewards deposit is not slippage protected and susceptible to MEV-attacks.

Proof of Concept

VotiumCoreStrategy.buyCvx() is not slippage protected, as even acknowledged by the comment in

ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
    value: _ethAmountIn
}(
    0,
    1,
    _ethAmountIn,
    0 // this is handled at the afEth level
);

This is handled at the afEth level only for deposits (when it is called via VotiumStrategy.deposit() by AfEth.deposit() which has a _minout), but NOT for rewards deposits.

Rewards are initiated by VotiumStrategyCore.applyRewards() which calls VotiumStrategyCore.depositRewards() either directly or via AfEth.depositRewards(). VotiumStrategyCore.depositRewards() then buyCvx(). None of these functions has a _minout or similar for slippage protection (it is sufficient to note that applyRewards() doesn't have such a parameter).

Recommended Mitigation Steps

Add a if (ethReceived < _minout) revert BelowMinOut() slippage protection in VotiumStrategyCore.applyRewards().

Assessed type

MEV

[c4-judge]

0xleastwood marked the issue as duplicate of #39

[elmutt]

@0xleastwood I dont think this is a dupe of 39

this ticket is talking about applyRewards(). 39 is talking about other functions

[0xleastwood]

@0xleastwood I dont think this is a dupe of 39

this ticket is talking about applyRewards(). 39 is talking about other functions

It's addressing the same function, the buyCvx(). Where the call is originating from is not important.

[elmutt]

My plan to fix this and #24 is pass _minOut to afEth.depositRewards() (in an upcoming PR).

Together with the fix for #31 (https://github.com/asymmetryfinance/afeth/pull/169) I think this issue will then be fixed

[0xleastwood]

Giving partial credit to this because it does not consider the case for withdraw()/sellCvx()

[c4-judge]

0xleastwood marked the issue as partial-50

[c4-judge]

0xleastwood marked the issue as duplicate of #23

[d3e4]

I mentioned the case for withdraw()/sellCvx() in #67 (QA) under L-07. I considered those of low severity since the user is not supposed to interact directly with VotiumStrategy, which implies that no security guarantees are given and thus none are violated. The case of buyCvx() is more severe because it involves a lack of slippage protection in AfEth. But even here the issue is not in buyCvx() itself. As @elmutt correctly points out, the focal point of the issue is applyRewards(), which is where the _minOut parameter must be supplied. The main issue #23 doesn't even mention the impact on AfEth, but rather the same warden reported this in #24.

[elmutt]

My plan to fix this and #24 is pass _minOut to afEth.depositRewards() (in an upcoming PR).

Together with the fix for #31 (asymmetryfinance/afeth#169) I think this issue will then be fixed

https://github.com/asymmetryfinance/afeth/pull/176 https://github.com/asymmetryfinance/afeth/pull/178

[0xleastwood]

Noted, restoring full credit to account for the added context outlined in the warden's QA report #67.

[c4-judge]

0xleastwood marked the issue as full credit

[c4-judge]

0xleastwood marked the issue as satisfactory