Lack of slippage protection for `depositRewards()` in `AfEth.sol` makes it susceptible to sandwich attacks

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L272-L293

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the buyCvx() function calls exchange_underlying() of Curve's ETH / CVX pool to buy CVX:

VotiumStrategyCore.sol#L233-L240

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

As seen from above, exchange_underlying() is called with its _min_dy parameter as 0, which means the minimum amount of CVX that the swap must return is effectively 0.

buyCvx() is used by VotiumStrategyCore's depositRewards() function to swap ETH gained from rewards to CVX:

VotiumStrategyCore.sol#L203-L204

    function depositRewards(uint256 _amount) public payable {
        uint256 cvxAmount = buyCvx(_amount);

This is called by the depositRewards() function in the AfEth contract:

AfEth.sol#L291

            votiumStrategy.depositRewards{value: amount}(amount);

However, the depositRewards() function in AfEth.sol, unlike deposit() and withdraw(), does not have a _minOut parameter or any other form of slippage protection. This leaves it susceptible to sandwich attacks.

Impact

As the depositRewards() function in AfEth.sol does not have any form of slippage protection, attackers can sandwich calls to depositRewards() to extract value from its buyCvx() call, resulting in a loss of rewards for the protocol.

Note that depositRewards() is automatically called whenever the rewarder of the VotiumStrategy contract calls applyRewards().

Proof of Concept

Consider the following scenario:

• Rewarder calls applyRewards() to distribute rewards to users. This calls depositRewards() in AfEth.sol to deposit ETH and exchange it to CVX.
• Alice sees his transaction in the mempool and front-runs it. She swaps a large amount of ETH into the Curve pool and gets CVX in return.
• Now, the rewarder's transaction is executed:
  • This calls depositRewards() in the VotiumStrategy contract with a portion of the deposited ETH.
  • buyCvx() attempts to swap the deposited ETH for CVX.
  • However, since the pool currently has a lot more ETH than CVX, buyCvx() only returns a small amount of CVX when called.
• Alice back-runs the transaction and swaps the CVX she gained back into ETH, which results in a profit for her.

In this scenario, Alice has sandwiched the call to depositRewards() to effectively steal a portion of rewards from the protocol.

Recommended Mitigation

Consider adding some form of slippage protection to depositRewards() in AfEth.sol. One way of achieving this would be to do the following:

1. Return cxvAmount when depositRewards() in VotiumStrategyCore is called:

VotiumStrategyCore.sol#L203-L208

-   function depositRewards(uint256 _amount) public payable {
-       uint256 cvxAmount = buyCvx(_amount);
+   function depositRewards(uint256 _amount) public payable returns (uint256 cvxAmount) {
+       cvxAmount = buyCvx(_amount);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        emit DepositReward(cvxPerVotium(), _amount, cvxAmount);
    }

2. Add a _minOut parameter in depositRewards(), which is the minimum amount of CVX that should be returned by the swap:

AfEth.sol#L272-L293

-   function depositRewards(uint256 _amount) public payable {
+   function depositRewards(uint256 _amount, uint256 _minOut) public payable {
        // Some code here...

        if (safEthRatio < ratio) {
            ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
        } else {
-           votiumStrategy.depositRewards{value: amount}(amount);
+           uint256 cvxAmount = votiumStrategy.depositRewards{value: amount}(amount);
+           if (cvxAmount < _minOut) revert BelowMinOut();
        }
    }

Assessed type

MEV

[c4-judge]

0xleastwood marked the issue as duplicate of #39

[elmutt]

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

its talking about different functions

[0xleastwood]

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

its talking about different functions

The vulnerable function is buyCvx(), regardless of what path it is being called from, they still seem to be equivalent issues.

[elmutt]

They are describing slightly different issues that can be solved in different ways.

In my opinion its kindof dangerous to just assume they will be solved by the same PR -- we have a PR ready on #39 but that PR is not sufficient to solve this issue.

I dont think our PR for #39 would fix this as the mitigations steps are different

[0xleastwood]

They are describing slightly different issues that can be solved in different ways.

In my opinion its kindof dangerous to just assume they will be solved by the same PR -- we have a PR ready on #39 but that PR is not sufficient to solve this issue.

I dont think our PR for #39 would fix this as the mitigations steps are different

Can I get access to the repo so I can look at these PRs?

[elmutt]

They are describing slightly different issues that can be solved in different ways. In my opinion its kindof dangerous to just assume they will be solved by the same PR -- we have a PR ready on #39 but that PR is not sufficient to solve this issue. I dont think our PR for #39 would fix this as the mitigations steps are different

Can I get access to the repo so I can look at these PRs?

let me double check really quick. now im second guessing my answer

[elmutt]

They are describing slightly different issues that can be solved in different ways. In my opinion its kindof dangerous to just assume they will be solved by the same PR -- we have a PR ready on #39 but that PR is not sufficient to solve this issue. I dont think our PR for #39 would fix this as the mitigations steps are different

Can I get access to the repo so I can look at these PRs?

let me double check really quick. now im second guessing my answer

Ok. after reviewing PR's I still think they are different. I will see about getting you access

[0xleastwood]

On second thoughts, I agree this is addressing the same issue but in a different area of the code.

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #23

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood marked the issue as partial-25

[0xleastwood]

Only giving 25% credit because it misses 2/3 edge cases where this should be applied.

[0xleastwood]

Duplicate issue by same warden as it's primary issue. depositRewards() was already covered in #23.

[c4-judge]

0xleastwood marked the issue as full credit

[c4-judge]

0xleastwood marked the issue as satisfactory

[elmutt]

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