Asymmetry Finance afETH Invitational - adriro's results

Lines of code

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

Vulnerability details

Summary

Withdrawals of amount zero from both SafEth and VotiumStrategy have issues downstream that will cause the transaction to revert, potentially bricking withdrawals from being executed.

Impact

Withdrawals in AfEth undergo a process to account for any potential delay when withdrawing locked tokens in the VotiumStrategy. When a withdrawal is requested, the implementation calculates the owed amounts for each token and queues the withdrawal. SafEth tokens will be reserved in the contract, and VotiumStrategy will also queue the withdrawal of CVX tokens.

When the time arrives, the user can call withdraw() to execute the withdrawal. This function will unstake from SafEth and withdraw from VotiumStrategy.

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

252:         ISafEth(SAF_ETH_ADDRESS).unstake(withdrawInfo.safEthWithdrawAmount, 0);
253:         AbstractStrategy(vEthAddress).withdraw(withdrawInfo.vEthWithdrawId);

Let's first consider the SafEth case. The current unstake() implementation in SafEth will revert if the unstaked amount is zero:

https://etherscan.io/address/0x591c4abf20f61a8b0ee06a5a2d2d2337241fe970#code#F1#L124

119:     function unstake(
120:         uint256 _safEthAmount,
121:         uint256 _minOut
122:     ) external nonReentrant {
123:         if (pauseUnstaking) revert UnstakingPausedError();
124:         if (_safEthAmount == 0) revert AmountTooLow();
125:         if (_safEthAmount > balanceOf(msg.sender)) revert InsufficientBalance();

As we can see in line 124, if _safEthAmount is zero the function will revert, and the transaction to withdraw() will revert too due to the bubbled error. This means that any withdrawal that ends up with a zero amount for SafEth will be bricked.

The VotiumStrategy case has a similar issue. The implementation of withdraw() will call sellCvx() to swap the owed amount of CVX for ETH. This is executed using a Curve pool, as we can see in the following snippet of code:

250:     function sellCvx(
251:         uint256 _cvxAmountIn
252:     ) internal returns (uint256 ethAmountOut) {
253:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
254:         // cvx -> eth
255:         uint256 ethBalanceBefore = address(this).balance;
256:         IERC20(CVX_ADDRESS).approve(CVX_ETH_CRV_POOL_ADDRESS, _cvxAmountIn);
257: 
258:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying(
259:             1,
260:             0,
261:             _cvxAmountIn,
262:             0 // this is handled at the afEth level
263:         );
264:         ethAmountOut = address(this).balance - ethBalanceBefore;
265:     }

If we drill down in the Curve implementation, we can see that it validates that the input amount is greater than zero:

https://etherscan.io/address/0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4#code#L714

709: def _exchange(sender: address, mvalue: uint256, i: uint256, j: uint256, dx: uint256, min_dy: uint256, use_eth: bool) -> uint256:
710:     assert not self.is_killed  # dev: the pool is killed
711:     assert i != j  # dev: coin index out of range
712:     assert i < N_COINS  # dev: coin index out of range
713:     assert j < N_COINS  # dev: coin index out of range
714:     assert dx > 0  # dev: do not exchange 0 coins

Again, this means that any withdrawal that ends up with a zero amount of vAfEth tokens (or the associated amount of CVX tokens) will be bricked when trying to execute the swap.

This can happen for different reasons. For example the current ratio may be 0 or 1e18, meaning the split goes entirely to SafEth or to VotiumStrategy. Another reason could be rounding, for small quantities the proportion may round down values to zero.

The critical issue is that both withdrawals are executed simultaneously. A zero amount shouldn't matter, but both happen at the time, and one may affect the other. If the SafEth amount is zero, it will brick the withdrawal for a potentially non-zero vAfEth amount. Similarly, if the vAfEth amount is zero, it will brick the withdrawal for a potentially non-zero SafEth amount

Proof of Concept

To simplify the case, let's say the current ratio is zero, meaning all goes to VotiumStrategy.

1. A user calls requestWithdraw(). Since currently the SafEth ratio is zero, the contract doesn't hold a position in SafEth. This means that safEthWithdrawAmount = 0, and the position is entirely in vAfEth (votiumWithdrawAmount > 0).
2. Time passes and the user can finally withdraw.
3. The user calls withdraw(). The implementation will try to call SafEth::unstake(0), which will cause an error, reverting the whole transaction.
4. The user will never be able to call withdraw(). Even if the ratios are changed, the calculated amount will be already stored in the withdrawIdInfo mapping. The withdrawal will be bricked, causing the loss of the vAfEth tokens.

Recommendation

For SafEth, avoid calling SafEth::unstake() if the calculated amount is zero:

+ if (withdrawInfo.safEthWithdrawAmount > 0) {
    ISafEth(SAF_ETH_ADDRESS).unstake(withdrawInfo.safEthWithdrawAmount, 0);
+ }

For VotiumStrategy, prevent requesting the withdrawal if votiumWithdrawAmount is zero, while also keeping track of this to also avoid executing the withdrawal when AfEth::withdraw() is called.

It is also recommended to add a guard in VotiumStrategy::withdraw() to avoid calling sellCvx() when cvxWithdrawAmount = 0.

-  uint256 ethReceived = sellCvx(cvxWithdrawAmount);
+  uint256 ethReceived = cvxWithdrawAmount > 0 ? sellCvx(cvxWithdrawAmount) : 0;

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L32

Vulnerability details

Summary

The current price implementation for the VotiumStrategy token uses a potentially invalid Chainlink response. This price is then used to calculate the price of AfEth and, subsequently, the amount of tokens to mint while depositing.

Impact

The price of VotiumStrategy tokens are determined by taking the amount of deposited CVX in the strategy, and multiplied by the current price of CVX in terms of ETH. This price is fetched using Chainlink in the ethPerCvx() function:

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L156-L186

156:     function ethPerCvx(bool _validate) public view returns (uint256) {
157:         ChainlinkResponse memory cl;
158:         try chainlinkCvxEthFeed.latestRoundData() returns (
159:             uint80 roundId,
160:             int256 answer,
161:             uint256 /* startedAt */,
162:             uint256 updatedAt,
163:             uint80 /* answeredInRound */
164:         ) {
165:             cl.success = true;
166:             cl.roundId = roundId;
167:             cl.answer = answer;
168:             cl.updatedAt = updatedAt;
169:         } catch {
170:             cl.success = false;
171:         }
172:         // verify chainlink response
173:         if (
174:             (!_validate ||
175:                 (cl.success == true &&
176:                     cl.roundId != 0 &&
177:                     cl.answer >= 0 &&
178:                     cl.updatedAt != 0 &&
179:                     cl.updatedAt <= block.timestamp &&
180:                     block.timestamp - cl.updatedAt <= 25 hours))
181:         ) {
182:             return uint256(cl.answer);
183:         } else {
184:             revert ChainlinkFailed();
185:         }
186:     }

As we can see from the previous snippet of code, if the _validate flag is off, then no validation is done, it can even return an uninitialized response from a failed call given the usage of the try/catch structure. This means that it can invalid price, stale price, or even zero when the call fails.

The VotiumStrategy price() function calls ethPerCvx(false), which means it carries forward any invalid CVX/ETH price.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L31-L33

31:     function price() external view override returns (uint256) {
32:         return (cvxPerVotium() * ethPerCvx(false)) / 1e18;
33:     }

The price of VotiumStrategy is then used in the AfEth contract to calculate its price and determine the amount of tokens to mint in deposit()

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

133:     function price() public view returns (uint256) {
134:         if (totalSupply() == 0) return 1e18;
135:         AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
136:         uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
137:             safEthBalanceMinusPending()) / 1e18;
138:         uint256 vEthValueInEth = (vEthStrategy.price() *
139:             vEthStrategy.balanceOf(address(this))) / 1e18;
140:         return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
141:     }

148:     function deposit(uint256 _minout) external payable virtual {
149:         if (pauseDeposit) revert Paused();
150:         uint256 amount = msg.value;
151:         uint256 priceBeforeDeposit = price();
152:         uint256 totalValue;
153: 
154:         AbstractStrategy vStrategy = AbstractStrategy(vEthAddress);
155: 
156:         uint256 sValue = (amount * ratio) / 1e18;
157:         uint256 sMinted = sValue > 0
158:             ? ISafEth(SAF_ETH_ADDRESS).stake{value: sValue}(0)
159:             : 0;
160:         uint256 vValue = (amount * (1e18 - ratio)) / 1e18;
161:         uint256 vMinted = vValue > 0 ? vStrategy.deposit{value: vValue}() : 0;
162:         totalValue +=
163:             (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
164:             (vMinted * vStrategy.price());
165:         if (totalValue == 0) revert FailedToDeposit();
166:         uint256 amountToMint = totalValue / priceBeforeDeposit;
167:         if (amountToMint < _minout) revert BelowMinOut();
168:         _mint(msg.sender, amountToMint);
169:     }

The VotiumStrategy price is first used in line 138 to calculate its TVL (vEthValueInEth). Any invalid price here will also mean an invalid price for AfEth.

Then both the AfEth price (line 151) and again the VotiumStrategy price (line 164) are used in deposit() to calculate the number of minted tokens. Depending on the direction of the wrong price, this means that the user will be minted more or less tokens than it should.

Proof of Concept

Let's suppose the Chainlink feed is stale and the current price of CVX/ETH has increased since then.

1. A user calls deposit() to create a new position in AfEth.
2. The function calculates the current price (priceBeforeDeposit) in order to know how many tokens should be minted.
3. The price() implementation will calculate the Votium strategy TVL using ethPerCvx(false), which will successfully return the stale price.
4. The price of AfEth will then be calculated using the old data, which will result in a lower value than the actual "real" price.
5. The user is minted tokens based on the incorrectly calculated priceBeforeDeposit, since this price is lower than the expected "real" price the user will be minted more tokens than expected.

Recommendation

Change the ethPerCvx() argument to true to make sure prices coming from Chainlink are correctly validated.

  function price() external view override returns (uint256) {
-     return (cvxPerVotium() * ethPerCvx(false)) / 1e18;
+     return (cvxPerVotium() * ethPerCvx(true)) / 1e18;
  }

Assessed type

Oracle

Lines of code

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

Vulnerability details

Summary

When withdrawals are enqueued in AfEth, the implementation will remove the tokens from the caller and lock these in the contract until the withdrawal is made effective. These tokens still count in the supply, and must not be considered during price calculation.

Impact

Withdrawals in AfEth undergo a process to account for any potential delay when withdrawing locked tokens in the VotiumStrategy. During this window of time, AfEth tokens are transferred from the caller into the contract. This can be seen in the implementation of requestWithdraw():

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

175:     function requestWithdraw(uint256 _amount) external virtual {
176:         uint256 withdrawTimeBefore = withdrawTime(_amount);
177:         if (pauseWithdraw) revert Paused();
178:         latestWithdrawId++;
179: 
180:         // ratio of afEth being withdrawn to totalSupply
181:         // we are transfering the afEth to the contract when we requestWithdraw
182:         // we shouldn't include that in the withdrawRatio
183:         uint256 afEthBalance = balanceOf(address(this));
184:         uint256 withdrawRatio = (_amount * 1e18) /
185:             (totalSupply() - afEthBalance);
186: 
187:         _transfer(msg.sender, address(this), _amount);
...

Line 187 transfers the tokens from the caller to the same AfEth contract, which are held until withdrawals are made effective, at which point the tokens are burned. Notice that these locked tokens are taken into account while calculating the withdrawRatio in line 184-185.

However, the same consideration is not present in the calculation of price():

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

133:     function price() public view returns (uint256) {
134:         if (totalSupply() == 0) return 1e18;
135:         AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
136:         uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
137:             safEthBalanceMinusPending()) / 1e18;
138:         uint256 vEthValueInEth = (vEthStrategy.price() *
139:             vEthStrategy.balanceOf(address(this))) / 1e18;
140:         return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
141:     }

As we can see in the previous snippet of code, the total supply of the token is referenced in lines 134 and 140 without subtracting the "locked" tokens held in the contract. This leads to different inconsistencies that will end up with an incorrect price value.

• If the single/last token holder requests a withdrawal, then those locked tokens are still counted in the supply. This means that the condition in line 134 will be false, where in reality it should be true since there is no circulating supply, and the return value of this function should be 1e18.
• When a holder requests a withdrawal, the implementation will reduce the balance of SafEth (by incrementing pendingSafEthWithdraws) and the balance of vAfEth (since those tokens are burned when VotiumStrategy::withdraw() is called). This means that calculation in line 140 will also be affected, vEthValueInEth and safEthValueInEth are calculated with the reduced position, but then those are divided by totalSupply(), which still counts for locked tokens.

Proof of Concept

1. User holds AfEth tokens and decides to withdraw by calling requestWithdraw().
2. Tokens are transferred from the user and locked into the contract. Total supply is not affected.
3. Corresponding token balances of SafEth and vAfEth are reduced in relation to the withdrawal. For SafEth, the pendingSafEthWithdraws is increased, making safEthBalanceMinusPending() account for this difference. For vAfEth, the balance held by the contract is reduced when the tokens are burned when VotiumStrategy::withdraw() is called.
4. At this point (and until the withdrawal is made effective when withdraw() is called), the implementation of price() will use the reduced balances to calculate safEthValueInEth and vEthValueInEth, but will still normalize them by totalSupply(), which considers the locked tokens in the contract.

Recommendation

The issue could be fixed by directly burning the tokens when requestWithdraw() is called, instead of transferring and locking them in the contract.

If these should still count in the totalSupply() for external reference while the withdrawal is pending, then the implementation of price() must factor them in the calculation:

    function price() public view returns (uint256) {
+       uint256 _totalSupply = totalSupply() - balanceOf(address(this));
-       if (totalSupply() == 0) return 1e18;
+       if (_totalSupply == 0) return 1e18;
        
        AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
        uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
            safEthBalanceMinusPending()) / 1e18;
        uint256 vEthValueInEth = (vEthStrategy.price() *
            vEthStrategy.balanceOf(address(this))) / 1e18;
-       return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
+       return ((vEthValueInEth + safEthValueInEth) * 1e18) / _totalSupply;
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L109

Vulnerability details

Summary

Direct deposits and withdrawals within VotiumStrategy lack any slippage controls, which opens up the possibility of sandwich attacks and Miner Extractable Value (MEV) exploits.

Impact

Interactions in the AfEth protocol often require the exchange of ETH for other assets. Users deposit ETH, and the protocol needs to convert it to other LSD tokens while staking in SafEth, and also to CVX tokens while depositing in the VotiumStrategy. Similarly, during withdrawals, the protocol converts those assets back to ETH.

While the AfEth contract implements slippage control in deposit() and withdraw() through a _minout parameter to ensure the return of a minimum number of assets after execution, the same is not true for the VotiumStrategy contract, which can be used in a standalone fashion.

The deposit() function in VotiumStrategy simple takes the received ETH amount and swaps it to CVX using buyCvx():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

39:     function deposit() public payable override returns (uint256 mintAmount) {
40:         uint256 priceBefore = cvxPerVotium();
41:         uint256 cvxAmount = buyCvx(msg.value);
42:         IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
43:         ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
44:         mintAmount = ((cvxAmount * 1e18) / priceBefore);
45:         _mint(msg.sender, mintAmount);
46:     }

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L227-L243

227:     function buyCvx(
228:         uint256 _ethAmountIn
229:     ) internal returns (uint256 cvxAmountOut) {
230:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
231:         // eth -> cvx
232:         uint256 cvxBalanceBefore = IERC20(CVX_ADDRESS).balanceOf(address(this));
233:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
234:             value: _ethAmountIn
235:         }(
236:             0,
237:             1,
238:             _ethAmountIn,
239:             0 // this is handled at the afEth level
240:         );
241:         uint256 cvxBalanceAfter = IERC20(CVX_ADDRESS).balanceOf(address(this));
242:         cvxAmountOut = cvxBalanceAfter - cvxBalanceBefore;
243:     }

In the previous snippet of code, line 239, we can see that the last argument to exchange_underlying() is zero, which is the minimum output amount. The comment reads "this is handled at the afEth level" which is partially true: it is only checked when being used from AfEth, but not for direct depositors of the VotiumStrategy.

The withdraw() function has the same issue when swapping CVX for ETH in sellCvx():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L109-L29

109:     function withdraw(uint256 _withdrawId) external override {
110:         if (withdrawIdToWithdrawRequestInfo[_withdrawId].owner != msg.sender)
111:             revert NotOwner();
112:         if (!this.canWithdraw(_withdrawId)) revert WithdrawNotReady();
113: 
114:         if (withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn)
115:             revert AlreadyWithdrawn();
116: 
117:         relock();
118: 
119:         uint256 cvxWithdrawAmount = withdrawIdToWithdrawRequestInfo[_withdrawId]
120:             .cvxOwed;
121: 
122:         uint256 ethReceived = sellCvx(cvxWithdrawAmount);
123:         cvxUnlockObligations -= cvxWithdrawAmount;
124:         withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn = true;
125: 
126:         // solhint-disable-next-line
127:         (bool sent, ) = msg.sender.call{value: ethReceived}("");
128:         if (!sent) revert FailedToSend();
129:     }

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L250-L265

250:     function sellCvx(
251:         uint256 _cvxAmountIn
252:     ) internal returns (uint256 ethAmountOut) {
253:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
254:         // cvx -> eth
255:         uint256 ethBalanceBefore = address(this).balance;
256:         IERC20(CVX_ADDRESS).approve(CVX_ETH_CRV_POOL_ADDRESS, _cvxAmountIn);
257: 
258:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying(
259:             1,
260:             0,
261:             _cvxAmountIn,
262:             0 // this is handled at the afEth level
263:         );
264:         ethAmountOut = address(this).balance - ethBalanceBefore;
265:     }

Again, in line 262 we see the same issue as before, the minimum output is configured to zero.

We can see that there is no slippage control at any level, the swap output amount is configured to zero, and there is no check of the number of minted tokens or the amount of ETH sent back to the user.

This implies that direct deposits or withdrawals in the VotiumStrategy contract are susceptible to arbitrary sandwich attacks by MEV bots, which could potentially result in a loss of funds for the user. In the case of a sandwiched deposit, there will be a reduced issuance of VotiumStrategy tokens, as the swap may yield fewer CVX tokens than anticipated. Similarly, a sandwiched withdrawal may lead to a reduced amount of ETH being returned to the user.

Proof of Concept

1. A user sends a transaction to deposit in VotiumStrategy.
2. A MEV bot sees the transaction, and swaps a large amount of ETH for CVX in the Curve Pool.
3. The user's transaction goes through and swap is executed in the manipulated pool.
4. The MEV bot swaps CVX back to ETH, and pockets the slippage difference.
5. The user gets less CVX tokens, which means less minted VotiumStrategy tokens.

Recommendation

Add a minimum output parameter to both deposit() and withdraw() in the VotiumStrategy contract.

The AfEth contract can still set these arguments as zero, since it already has slippage control, while allowing direct users of the VotiumStrategy contract to protect against MEV attacks.

-   function deposit() public payable override returns (uint256 mintAmount) {
+   function deposit(uint256 minOut) public payable override returns (uint256 mintAmount) {
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
+       require(mintAmount >= minOut);
        _mint(msg.sender, mintAmount);
    }

-   function withdraw(uint256 _withdrawId) external override {
+   function withdraw(uint256 _withdrawId, uint256 minOut) external override {
        if (withdrawIdToWithdrawRequestInfo[_withdrawId].owner != msg.sender)
            revert NotOwner();
        if (!this.canWithdraw(_withdrawId)) revert WithdrawNotReady();

        if (withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn)
            revert AlreadyWithdrawn();

        relock();

        uint256 cvxWithdrawAmount = withdrawIdToWithdrawRequestInfo[_withdrawId]
            .cvxOwed;

        uint256 ethReceived = sellCvx(cvxWithdrawAmount);
        cvxUnlockObligations -= cvxWithdrawAmount;
        withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn = true;

+       require(ethReceived >= minOut);

        // solhint-disable-next-line
        (bool sent, ) = msg.sender.call{value: ethReceived}("");
        if (!sent) revert FailedToSend();
    }

Assessed type

MEV

Lines of code

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

Vulnerability details

Summary

Deposits to SafEth and VotiumStrategy coming from rewards lack slippage control, making them susceptible to sandwich attacks by MEV bots, which can result in a loss of funds for the protocol.

Impact

Rewards coming from the VotiumStrategy contract are compounded and deposited back to the protocol using the depositRewards() function:

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

272:     function depositRewards(uint256 _amount) public payable {
273:         IVotiumStrategy votiumStrategy = IVotiumStrategy(vEthAddress);
274:         uint256 feeAmount = (_amount * protocolFee) / 1e18;
275:         if (feeAmount > 0) {
276:             // solhint-disable-next-line
277:             (bool sent, ) = feeAddress.call{value: feeAmount}("");
278:             if (!sent) revert FailedToSend();
279:         }
280:         uint256 amount = _amount - feeAmount;
281:         uint256 safEthTvl = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
282:             safEthBalanceMinusPending()) / 1e18;
283:         uint256 votiumTvl = ((votiumStrategy.cvxPerVotium() *
284:             votiumStrategy.ethPerCvx(true)) *
285:             IERC20(vEthAddress).balanceOf(address(this))) / 1e36;
286:         uint256 totalTvl = (safEthTvl + votiumTvl);
287:         uint256 safEthRatio = (safEthTvl * 1e18) / totalTvl;
288:         if (safEthRatio < ratio) {
289:             ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
290:         } else {
291:             votiumStrategy.depositRewards{value: amount}(amount);
292:         }
293:     }

As we can see in lines 289 and 291, depending on the current TVL ratio between both collaterals, rewards will be either staked in SafEth or deposited back to VotiumStrategy.

Both scenarios experience the same lack of slippage control. The call to stake() sets zero as the _minOut argument in SafEth. Similarly, in the depositRewards() function of VotiumStrategy, the process involves using a Curve Pool to exchange ETH for CVX tokens with a hardcoded min_dy parameter set to zero.

In any case, the reward compounding transaction could be sandwiched by a MEV bot, causing less output received and a loss of funds to the protocol in general.

Recommendation

Add slippage control to the deposit rewards flow in order to allow the rewarded role to specify a minimum output of assets when rewards are either staked in SafEth, or swapped for CVX in VotiumStrategy.

Assessed type

MEV

Lines of code

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

Vulnerability details

Summary

The AfEth ratio between the collaterals can be modified but there is no direct way to balance the assets to follow the new ratio.

Impact

The AfEth contract contains a configurable parameter ratio that indicates the intended balance between the two collaterals, SafEth and the Votium strategy. For example, a value of 3e17 means that 30% of the TVL should go to SafEth, and 70% should go to Votium.

This ratio is followed when new deposits are made. The deposited ETH is splitted according to the ratio and channeled in proportion to each collateral. The ratio is also checked when rewards are deposited to direct them to the proper collateral.

The ratio can also be modified by the admins of the protocol using the setRatio() function.

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

90:     function setRatio(uint256 _newRatio) public onlyOwner {
91:         ratio = _newRatio;
92:     }

However, there is no way to balance the assets once a new ratio is defined. The implementation will need to rely on new deposits and reward compounding to slowly correct the offset, which may take a lot of time and may be impractical for most cases.

Proof of Concept

Let's assume the protocol is deployed with a ratio of 3e17.

1. Deposits follow the configured ratio and split the TVL in 30% for SafEth and 70% for Votium.
2. At some point, the protocol decides to switch to 50%-50% and sets a new ratio of 5e17.
3. New deposits will follow the new ratio and split 50% for each collateral, but these have potentially accumulated a large amount of TVL with the previous split. The existing difference will continue, new deposits can't correct this offset.

Recommendation

Similar to how it is done in SafEth, the AfEth contract could have a rebalancing function which withdraws the proper amount from one collateral and deposits it in the other collateral, in order to correct the offset and target the new configured ratio. This function should be admin controlled, and support slippage control to correctly handle potentially large amounts of swaps.

An alternative could be to also correct a potential deviation in the ratio using new deposits. This could help speed up the correction by not only relying on rewards, but will also endure a delay in the convergence.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

While the intention is to use the 0x protocol to sell rewards, the implementation doesn't provide any basic guarantee this will correctly happen and grants the rewarder arbitrary control over the tokens held by the strategy.

Impact

Rewards earned in the VotingStrategy contract are exchanged for ETH and deposited back into the protocol. As indicated by the documentation, the intention is to swap these rewards for ETH using the 0x protocol:

Votium rewards are claimed with claimRewards() using merkle proofs published by votium every 2 weeks. applyRewards() sells rewards on 0x and deposits them back into afEth (and ultimately back into the safEth & votium strategies), making the afEth price go up.

However, the implementation of applyRewards() shallowly executes a series of calls to arbitrary targets:

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

272:     function applyRewards(SwapData[] calldata _swapsData) public onlyRewarder {
273:         uint256 ethBalanceBefore = address(this).balance;
274:         for (uint256 i = 0; i < _swapsData.length; i++) {
275:             // Some tokens do not allow approval if allowance already exists
276:             uint256 allowance = IERC20(_swapsData[i].sellToken).allowance(
277:                 address(this),
278:                 address(_swapsData[i].spender)
279:             );
280:             if (allowance != type(uint256).max) {
281:                 if (allowance > 0) {
282:                     IERC20(_swapsData[i].sellToken).approve(
283:                         address(_swapsData[i].spender),
284:                         0
285:                     );
286:                 }
287:                 IERC20(_swapsData[i].sellToken).approve(
288:                     address(_swapsData[i].spender),
289:                     type(uint256).max
290:                 );
291:             }
292:             (bool success, ) = _swapsData[i].swapTarget.call(
293:                 _swapsData[i].swapCallData
294:             );
295:             if (!success) {
296:                 emit FailedToSell(_swapsData[i].sellToken);
297:             }
298:         }
299:         uint256 ethBalanceAfter = address(this).balance;
300:         uint256 ethReceived = ethBalanceAfter - ethBalanceBefore;
301: 
302:         if (address(manager) != address(0))
303:             IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
304:         else depositRewards(ethReceived);
305:     }

This not only fails to provide any guarantee that 0x will be used (and that it will be used correctly), but grants a lot of power to the rewarder which can be used accidentally or purposely to negatively impact the protocol. The rewarded role can grant any token approval to any spender and execute arbitrary calls on behalf of the VotingStrategy.

Recommendation

Provide better guarantees in the implementation of applyRewards() that 0x will be used to swap rewards, to ensure a more transparent and less error prone solution.

• Instead of granting arbitrary allowance to any spender, set this to the 0x entrypoint.
• Change arbitrary calls to the 0x protocol entrypoint.
• Data sent to the 0x contract could also be validated, for example to ensure the output token is ETH.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L135-L149

Vulnerability details

Summary

The VotiumStrategy withdrawal process involves relocking CVX tokens, which can potentially lead to a denial of service and loss of user funds if the underlying vlCVX contract is shutdown.

Impact

When withdrawals are executed in VotiumStrategy, the implementation of withdraw() will call relock() in order to relock any available excess (i.e. expired tokens minus the pending obligations) of CVX tokens to lock them again in the Convex protocol.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L135-L149

135:     function relock() public {
136:         (, uint256 unlockable, , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
137:             address(this)
138:         );
139:         if (unlockable > 0)
140:             ILockedCvx(VLCVX_ADDRESS).processExpiredLocks(false);
141:         uint256 cvxBalance = IERC20(CVX_ADDRESS).balanceOf(address(this));
142:         uint256 cvxAmountToRelock = cvxBalance > cvxUnlockObligations
143:             ? cvxBalance - cvxUnlockObligations
144:             : 0;
145:         if (cvxAmountToRelock > 0) {
146:             IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
147:             ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
148:         }
149:     }

This seems fine at first, but if we dig into the implementation of lock() we can see that the preconditions of this function requires the contract not to be shutdown:

https://etherscan.io/address/0x72a19342e8F1838460eBFCCEf09F6585e32db86E#code#L1469

521:     function _lock(address _account, uint256 _amount, uint256 _spendRatio, bool _isRelock) internal {
522:         require(_amount > 0, "Cannot stake 0");
523:         require(_spendRatio <= maximumBoostPayment, "over max spend");
524:         require(!isShutdown, "shutdown");

Which means that any call to lock() after the contract is shutdown will revert. This is particularly bad because relock() is called as part of the withdraw process. If the vlCVX contract is shutdown, VotiumStrategy depositors won't be able to withdraw their position, causing a potential loss of funds.

Note that this also will cause a DoS while depositing rewards, since depositRewards() in VotiumStrategy also calls ILockedCvx::lock().

Proof of Concept

1. vlCVX contract is shutdown.
2. A user requests a withdrawal using requestWithdrawal()
3. The user calls withdraw() when the withdrawal epoch is reached.
4. The implementation will call relock(), which will call ILockedCvx::lock().
5. The implementation of lock() will throw an error because the vault is already shutdown.
6. The transaction will be reverted.

Recommendation

Add a condition to check if the contract is shutdown to avoid the call to lock() and the potential denial of service.

    function relock() public {
        (, uint256 unlockable, , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
            address(this)
        );
        if (unlockable > 0)
            ILockedCvx(VLCVX_ADDRESS).processExpiredLocks(false);
        uint256 cvxBalance = IERC20(CVX_ADDRESS).balanceOf(address(this));
        uint256 cvxAmountToRelock = cvxBalance > cvxUnlockObligations
            ? cvxBalance - cvxUnlockObligations
            : 0;
-       if (cvxAmountToRelock > 0) {
+       if (cvxAmountToRelock > 0 && !ILockedCvx(VLCVX_ADDRESS).isShutdown()) {
            IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmountToRelock);
            ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmountToRelock, 0);
        }
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L54-L103

Vulnerability details

Summary

Withdrawals in VotiumStrategy are executed in queue since CVX tokens are potentially locked in Convex. However, the implementation fails to consider the case where unlocked assets are already enough to cover the withdrawal, leading to different issues.

Impact

VotiumStrategy withdrawals are executed in queue since the underlying CVX tokens may be locked in the Convex platform. Depositors must request a withdrawal and wait in queue until the epoch associated with their withdrawal is reached in order to exit their position. The core of this logic is present in the function requestWithdraw():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L54-L103

054:     function requestWithdraw(
055:         uint256 _amount
056:     ) public override returns (uint256 withdrawId) {
057:         latestWithdrawId++;
058:         uint256 _priceInCvx = cvxPerVotium();
059: 
060:         _burn(msg.sender, _amount);
061: 
062:         uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
063:             block.timestamp
064:         );
065:         (
066:             ,
067:             uint256 unlockable,
068:             ,
069:             ILockedCvx.LockedBalance[] memory lockedBalances
070:         ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
071:         uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
072:         cvxUnlockObligations += cvxAmount;
073: 
074:         uint256 totalLockedBalancePlusUnlockable = unlockable +
075:             IERC20(CVX_ADDRESS).balanceOf(address(this));
076: 
077:         for (uint256 i = 0; i < lockedBalances.length; i++) {
078:             totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
079:             // we found the epoch at which there is enough to unlock this position
080:             if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
081:                 (, uint32 currentEpochStartingTime) = ILockedCvx(VLCVX_ADDRESS)
082:                     .epochs(currentEpoch);
083:                 uint256 timeDifference = lockedBalances[i].unlockTime -
084:                     currentEpochStartingTime;
085:                 uint256 epochOffset = timeDifference /
086:                     ILockedCvx(VLCVX_ADDRESS).rewardsDuration();
087:                 uint256 withdrawEpoch = currentEpoch + epochOffset;
088:                 withdrawIdToWithdrawRequestInfo[
089:                     latestWithdrawId
090:                 ] = WithdrawRequestInfo({
091:                     cvxOwed: cvxAmount,
092:                     withdrawn: false,
093:                     epoch: withdrawEpoch,
094:                     owner: msg.sender
095:                 });
096: 
097:                 emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
098:                 return latestWithdrawId;
099:             }
100:         }
101:         // should never get here
102:         revert InvalidLockedAmount();
103:     }

The implementation first considers available tokens that should be ready to be withdrawn. Line 74-75 sets totalLockedBalancePlusUnlockable to the amount of unlockable tokens (expired tokens in Convex that can be withdrawn) plus any available CVX balance in the contract.

However, the implementation fails to consider that this available balance may be already enough to cover the withdrawal, and proceeds to search within the locked balances by epoch. This will lead to different issues:

• A user that requests a withdrawal of an amount such that there is enough available balance to cover for it, will still need to wait until the end of the next locked cycle. The implementation will start searching the locked balances array and stop at the first match, and set the withdrawal epoch as the end of the matched period. Even if there are enough tokens to cover for the withdrawal, the user is forced into an unnecessary delay.
• If there are none locked balances, meaning everything is already in an unlockable state, the implementation will cause a denial of service. The for loop in line 77 won't be executed and the execution will be reverted due to the revert in line 102.

Proof of Concept

Let's illustrate the denial of service case. We assume all deposits in VotiumStrategy were done before the 16 weeks period, which means all vlCVX should be in an unlocked state.

1. User calls requestWithdraw(amount).
2. The implementation fetches current position from vlCVX contract using ILockedCvx.lockedBalances(). This will return the entire position as unlockable and an empty array for lockedBalances.
3. The function sets totalLockedBalancePlusUnlockable as unlockable + IERC20(CVX_ADDRESS).balanceOf(address(this)). This should be enough to cover the requested amount by the user.
4. The implementation continues to search through the lockedBalances, but since this array is empty the for loop is never executed.
5. The function reaches the end and is reverted with a InvalidLockedAmount() error.

Recommendation

Before searching through the lockedBalances array, check if there available unlocked tokens are enough to cover the withdrawal. If so, the withdrawal can be set for the current epoch. This will fix the unnecessary delay and the potential denial of service.

    function requestWithdraw(
        uint256 _amount
    ) public override returns (uint256 withdrawId) {
        latestWithdrawId++;
        uint256 _priceInCvx = cvxPerVotium();

        _burn(msg.sender, _amount);

        uint256 currentEpoch = ILockedCvx(VLCVX_ADDRESS).findEpochId(
            block.timestamp
        );
        (
            ,
            uint256 unlockable,
            ,
            ILockedCvx.LockedBalance[] memory lockedBalances
        ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(address(this));
        uint256 cvxAmount = (_amount * _priceInCvx) / 1e18;
        cvxUnlockObligations += cvxAmount;

        uint256 totalLockedBalancePlusUnlockable = unlockable +
            IERC20(CVX_ADDRESS).balanceOf(address(this));
        
+       if (totalLockedBalancePlusUnlockable >= amount) {
+           withdrawIdToWithdrawRequestInfo[
+               latestWithdrawId
+           ] = WithdrawRequestInfo({
+               cvxOwed: cvxAmount,
+               withdrawn: false,
+               epoch: currentEpoch,
+               owner: msg.sender
+           });
+           emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
+           return latestWithdrawId;
+       }

        for (uint256 i = 0; i < lockedBalances.length; i++) {
            totalLockedBalancePlusUnlockable += lockedBalances[i].amount;
            // we found the epoch at which there is enough to unlock this position
            if (totalLockedBalancePlusUnlockable >= cvxUnlockObligations) {
                (, uint32 currentEpochStartingTime) = ILockedCvx(VLCVX_ADDRESS)
                    .epochs(currentEpoch);
                uint256 timeDifference = lockedBalances[i].unlockTime -
                    currentEpochStartingTime;
                uint256 epochOffset = timeDifference /
                    ILockedCvx(VLCVX_ADDRESS).rewardsDuration();
                uint256 withdrawEpoch = currentEpoch + epochOffset;
                withdrawIdToWithdrawRequestInfo[
                    latestWithdrawId
                ] = WithdrawRequestInfo({
                    cvxOwed: cvxAmount,
                    withdrawn: false,
                    epoch: withdrawEpoch,
                    owner: msg.sender
                });

                emit WithdrawRequest(msg.sender, cvxAmount, latestWithdrawId);
                return latestWithdrawId;
            }
        }
        // should never get here
        revert InvalidLockedAmount();
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L203

Vulnerability details

Summary

The reward system in VotiumStrategy can be potentially gamed by users to enter just before rewards are deposited and request an exit after that. Depending on the withdrawal queue, users may exit as early as the next epoch and avoid waiting the normal 16 weeks of vote locked CVX.

Impact

Voting in the Convex protocol requires a commitment of at least 16 weeks. Holders of CVX tokens can lock their tokens into vlCVX, which grants them voting power in Curve gauges.

The same mechanism is applied internally in the VotiumStrategy contract. Deposited ETH is swapped to CVX and locked for vlCVX. Withdrawals are executed in a queued fashion, by reserving tokens that will eventually expire in coming epochs. A user exiting the strategy may have enough tokens to exit their position as early as the next epoch.

This means that, under the right circumstances, a user may deposit in VotiumStrategy and withdraw from it in a short period of time. The user just needs to have available expirable tokens coming from previous deposits in the platform, not necessarily related to the ones coming from their deposit. This can potentially reduce the commitment, requiring much less time than the required 16 weeks when using Convex directly.

This would allow users to game the system and enter the protocol just to collect the rewards, with a minimal commitment in the platform.

Proof of Concept

Let's say an attacker is anticipating the claiming of rewards in VotiumStrategy, and let's assume also that there are enough tokens that will be expiring in the next epoch to sufficiently cover their position.

1. The attacker deposits into the strategy just before rewards are claimed.
2. Rewarder claims rewards and deposits them back into the strategy, increasing the value for holders.
3. Right after that, the attacker requests a withdrawal. Since there are enough expirable tokens, the withdrawal is queued for the next epoch.
4. The attacker just needs to wait for the next epoch to exit their position, along with the rewards.

Recommendation

This is a variation of a common attack in vaults that compound rewards, present in different yield protocols. The usual mitigation is to introduce some delay or penalty to avoid bad intentionally users from depositing just to earn the rewards and leave.

In this case, two possible solutions are:

• Introduce some kind of minimum permanency delay for depositors. This could be the 16 weeks defined by Convex, or a fraction of it to be more flexible, e.g. 4 weeks.
• Stream rewards over a period of time. Instead of just depositing back the rewards as an immediate increase of value, have these rewards be linearly unlocked over a period of time. This will cause depositors to stay within the protocol to collect the rewards.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

AfEth main actions execute on-chain swaps and lack an expiration deadline, which enables pending transactions to be maliciously executed at a later point.

Impact

Both AfEth deposits and withdrawals include on-chain swaps in AMM protocols as part of their execution, in order to convert the deposited ETH into the different underlying assets held by SafEth and the Votium strategy.

In the case of SafEth, depending on the derivative, staking may involve swapping ETH for other LSD. For example, the RocketPool derivative implementation uses Balancer to swap between ETH and rETH during deposits:

https://etherscan.io/address/0xb3e64c481f0fc82344a7045592284fddb9905b8b#code#F1#L157

function deposit() external payable onlyOwner returns (uint256) {
    uint256 rethBalanceBefore = IERC20(rethAddress()).balanceOf(
        address(this)
    );
    balancerSwap(msg.value);
    uint256 received = IERC20(rethAddress()).balanceOf(address(this)) -
        rethBalanceBefore;
    underlyingBalance = super.finalChecks(
        ethPerDerivative(true),
        msg.value,
        maxSlippage,
        received,
        true,
        underlyingBalance
    );
    return received;
}

In the case of Votium, deposited ETH is swapped to CVX in order to lock it in Convex. Similarly, when withdrawing, CVX tokens are swapped back to ETH. This is done using a Curve Pool in the buyCvx() and sellCvx() functions:

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L227-L265

227:     function buyCvx(
228:         uint256 _ethAmountIn
229:     ) internal returns (uint256 cvxAmountOut) {
230:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
231:         // eth -> cvx
232:         uint256 cvxBalanceBefore = IERC20(CVX_ADDRESS).balanceOf(address(this));
233:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
234:             value: _ethAmountIn
235:         }(
236:             0,
237:             1,
238:             _ethAmountIn,
239:             0 // this is handled at the afEth level
240:         );
241:         uint256 cvxBalanceAfter = IERC20(CVX_ADDRESS).balanceOf(address(this));
242:         cvxAmountOut = cvxBalanceAfter - cvxBalanceBefore;
243:     }
244: 
245:     /**
246:      * @notice - Internal utility function to sell cvx for eth
247:      * @param _cvxAmountIn - Amount of cvx to sell
248:      * @return ethAmountOut - Amount of eth received
249:      */
250:     function sellCvx(
251:         uint256 _cvxAmountIn
252:     ) internal returns (uint256 ethAmountOut) {
253:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
254:         // cvx -> eth
255:         uint256 ethBalanceBefore = address(this).balance;
256:         IERC20(CVX_ADDRESS).approve(CVX_ETH_CRV_POOL_ADDRESS, _cvxAmountIn);
257: 
258:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying(
259:             1,
260:             0,
261:             _cvxAmountIn,
262:             0 // this is handled at the afEth level
263:         );
264:         ethAmountOut = address(this).balance - ethBalanceBefore;
265:     }

While both actions in AfEth, deposit() and withdraw(), have a minimum output parameter to control slippage, this doesn't offer protection against when the transaction is actually executed. If the price of the underlying assets drops while the transaction is pending, then the minimum output can still be fulfilled, but the user will get a bad rate due to the stale price. The outdated slippage value now allows for a high slippage trade in detriment of the user.

This can be also attacked by MEV bots which can still sandwich the transaction to profit on the difference. See this issue for an excellent reference on the topic (the author runs a MEV bot).

Proof of Concept

1. A user submits a transaction to deposit in AfEth.
2. The transaction sits in the mempool without being included in a block.
3. The price of CVX/ETH drops.
4. The transaction gets executed by the blockchain.
5. Since the price of CVX has dropped, the user will still get the minimum output expected but this will still represent less tokens than it would expect since the transaction has been delayed and the original price is now stale.

Recommendation

Add a deadline timestamp to the deposit() and withdraw() functions, and revert if this timestamp has passed.

Note also that the same should be applied to the VotiumStrategy contract if deposits and withdrawals are made directly there, without going through AfEth.

-   function deposit(uint256 _minout) external payable virtual {
+   function deposit(uint256 _minout, uint256 deadline) external payable virtual {
            if (pauseDeposit) revert Paused();
+           if (block.timestamp > deadline) revert StaleAction();

    function withdraw(
        uint256 _withdrawId,
-       uint256 _minout
+       uint256 _minout,
+       uint256 deadline
    ) external virtual onlyWithdrawIdOwner(_withdrawId) {
        if (pauseWithdraw) revert Paused();
+       if (block.timestamp > deadline) revert StaleAction();

Assessed type

MEV

Lines of code

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

Vulnerability details

Summary

Some functions that are part of the Votium reward flow are left unprotected and can be accessed by anyone to spend resources held by the contract.

Impact

Rewards coming from the Votium protocol are claimed and compounded back in AfEth. This flow consists of two parts, both controlled and initiated by the rewarder role: first, rewards are claimed in Votium and Convex using claimRewards(), second, those rewards are swapped to ETH and deposited back in the protocol using applyRewards().

After rewards are swapped, the VotiumStrategy will call AfEth to manage the deposited rewards, which eventually calls back the VotiumStrategy. These interactions are represented in the previous diagram as steps 5 and 6.

However, both of the functions that implement these steps are publicly accessible and don't have any validation over the amount of ETH sent. Let's first see the case of AfEth::depositRewards():

272:     function depositRewards(uint256 _amount) public payable {
273:        IVotiumStrategy votiumStrategy = IVotiumStrategy(vEthAddress);
274:         uint256 feeAmount = (_amount * protocolFee) / 1e18;
275:         if (feeAmount > 0) {
276:             // solhint-disable-next-line
277:             (bool sent, ) = feeAddress.call{value: feeAmount}("");
278:             if (!sent) revert FailedToSend();
279:         }
280:         uint256 amount = _amount - feeAmount;
281:         uint256 safEthTvl = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
282:             safEthBalanceMinusPending()) / 1e18;
283:         uint256 votiumTvl = ((votiumStrategy.cvxPerVotium() *
284:             votiumStrategy.ethPerCvx(true)) *
285:             IERC20(vEthAddress).balanceOf(address(this))) / 1e36;
286:         uint256 totalTvl = (safEthTvl + votiumTvl);
287:         uint256 safEthRatio = (safEthTvl * 1e18) / totalTvl;
288:         if (safEthRatio < ratio) {
289:             ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
290:         } else {
291:             votiumStrategy.depositRewards{value: amount}(amount);
292:         }
293:     }

As we can see in the previous snippet of code, the function doesn't have any access control and doesn't check if the _amount parameter matches the amount of ETH being sent (msg.value). Anyone can call this function with any amount value without actually sending any ETH value.

The implementation of depositRewards() in VotiumStrategyCore has the same issue:

203:     function depositRewards(uint256 _amount) public payable {
204:         uint256 cvxAmount = buyCvx(_amount);
205:         IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
206:         ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
207:         emit DepositReward(cvxPerVotium(), _amount, cvxAmount);
208:     }

Any ETH held in these two contracts can be arbitrarily spent by any unauthorized account. The caller cannot remove value from here, unless sandwiching the trade or benefitting via a third-party call, but can use these functions to grief and unauthorizedly spend any ETH present in these contracts.

Recommendation

If these functions are indeed intended to be publicly accessible, then add a validation to ensure that the amount argument matches the callvalue sent, i.e. require(_amount == msg.value).

On the other hand, if these should only be part of the reward flow initiated by the rewarder role, then validate that AfEth::depositRewards() is called from the Votium Strategy (vEthAddress), and validate that VotiumStrategy::depositRewards() is called either from AfEth (manager) or internally through applyRewards().

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39

Vulnerability details

Summary

The VotiumStrategy contract is susceptible to the Inflation Attack, in which the first depositor can be front-runned by an attacker to steal their deposit.

Impact

Both AfEth and VotiumStrategy acts as vaults: accounts deposit some tokens and get back another token (share) that represents their participation in the vault.

These types of contracts are potentially vulnerable to the inflation attack: an attacker can front-run the initial deposit to the vault to inflate the value of a share and render the front-runned deposit worthless.

In AfEth, this is successfully mitigated by the slippage control. Any attack that inflates the value of a share to decrease the number of minted shares is rejected due to the validation of minimum output:

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

166:         uint256 amountToMint = totalValue / priceBeforeDeposit;
167:         if (amountToMint < _minout) revert BelowMinOut();

However, this is not the case of VotiumStrategy. In this contract, no validation is done in the number of minted tokens. This means that an attacker can execute the attack by front-running the initial deposit, which may be from AfEth or from any other account that interacts with the contract. See Proof of Concept for a detailed walkthrough of the issue.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

39:     function deposit() public payable override returns (uint256 mintAmount) {
40:         uint256 priceBefore = cvxPerVotium();
41:         uint256 cvxAmount = buyCvx(msg.value);
42:         IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
43:         ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
44:         mintAmount = ((cvxAmount * 1e18) / priceBefore);
45:         _mint(msg.sender, mintAmount);
46:     }

Proof of Concept

Let's say a user wants to deposit in VotiumStrategy and calls deposit() sending an ETH amount such as it is expected to buy X tokens of CVX. Attacker will front-run the transaction and execute the following:

1. Initial state is empty contract, assets = 0 and supply = 0.
2. Attacker calls deposit with an amount of ETH such as to buy 1e18 CVX tokens, this makes assets = 1e18 and supply = 1e18.
3. Attacker calls requestWithdraw(1e18 - 1) so that supply = 1, assume also 1e18 - 1 CVX tokens are withdrawn so that cvxUnlockObligations = 1e18 - 1.
4. Attacker transfers (donates) X amount of CVX to VotiumStrategy contract.
5. At this point, priceBefore = cvxPerVotium() = (totalCvx - cvxUnlockObligations) * 1e18 / supply = (X + 1e18 - (1e18 - 1)) * 1e18 / 1 = (X + 1) * 1e18
6. User transaction gets through and deposit() buys X amount of CVX. Minted tokens will be mintAmount = X * 1e18 / priceBefore = X * 1e18 / (X + 1) * 1e18 = X / (X + 1) = 0.
7. User is then minted zero VotiumStrategy tokens.
8. Attacker calls requestWithdraw() again to queue withdrawal to remove all CVX balance from the contract, including the tokens deposited by the user.

Recommendation

There are multiple ways of solving the issue:

1. Similar to AfEth, add a minimum output check to ensure the amount of minted shares.
2. Track asset balances internally so an attacker cannot donate assets to inflate shares.
3. Mint an initial number of "dead shares", similar to how UniswapV2 does.

A very good discussion of these can be found here.

Assessed type

Other