Lack of access control and value validation in the reward flow exposes functions to public access

[c4-submissions]

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

[elmutt]

@toshiSat I believe best solution is to get rid of _amount and only use msg.value here

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #33

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[romeroadrian]

@0xleastwood Liam, sorry, may I ask why this issue was downgraded? It seems it was linked somehow to #33 which got downgraded recently. The exposed functions here are part of both core contracts.

[0xleastwood]

@0xleastwood Liam, sorry, may I ask why this issue was downgraded? It seems it was linked somehow to #33 which got downgraded recently. The exposed functions here are part of both core contracts.

Oops, I have no idea what happened here. This was not meant to be downgraded. Apologies.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[0xleastwood]

Oh wait this was a dupe of an issue I downgraded. Let me confirm.

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood marked the issue as primary issue

[d3e4]

@0xleastwood I mentioned this issue in L-08.

However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.

[0xleastwood]

@0xleastwood I mentioned this issue in L-08.

However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.

These are not equivalent, you talk about the fact that _amount == msg.value should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, the depositRewards() function is missing access control.

[d3e4]

@0xleastwood I mentioned this issue in L-08.
However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.

These are not equivalent, you talk about the fact that _amount == msg.value should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, the depositRewards() function is missing access control.

And what is the impact according to #38?
The impact of not checking that _amount == msg.value is that "any discrepancy is lost".
Otherwise, there is no ether in the contracts, so calling them does nothing. But if someone for some reason sends ether to the contract, then it is beneficial to the protocol if these functions are called so that the ether is deposited as rewards. Why should the public not be able to do this, which only benefits the protocol, on behalf of the owner? So what is the impact of the missing access control?

[romeroadrian]

@0xleastwood I mentioned this issue in L-08.
However, note that the contracts do not hold any ETH (except dust or if sent there by mistake) so calling these functions directly wouldn't really do anything. And if there is any ETH in the contract it is just deposited as rewards, which is the only way it should be spent; it cannot be made to send it elsewhere.

These are not equivalent, you talk about the fact that _amount == msg.value should be checked but fail to highlight the impact of not having this check. Plus this issue is more than that, the depositRewards() function is missing access control.

And what is the impact according to #38? The impact of not checking that _amount == msg.value is that "any discrepancy is lost". Otherwise, there is no ether in the contracts, so calling them does nothing. But if someone for some reason sends ether to the contract, then it is beneficial to the protocol if these functions are called so that the ether is deposited as rewards. Why should the public not be able to do this, which only benefits the protocol, on behalf of the owner? So what is the impact of the missing access control?

I'm not stating any direct theft of funds (in fact it is mentioned in the report), hence the med severity. It's the combination of both issues that conflicts with the protocol specs:

depositRewards() is called by the votium strategy upon claiming rewards to make the afEth price go up by distributing funds into both strategies according to ratio.

Rewarder - Address of wallet that will handle calling the reward functions and swapping the tokens out

[d3e4]

It's the combination of both issues that conflicts with the protocol specs:

Conflicting with protocol specs is QA.

[romeroadrian]

It's the combination of both issues that conflicts with the protocol specs:

Conflicting with protocol specs is QA.

No, it's medium

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[0xleastwood]

Agree, it is medium.

[d3e4]

@romeroadrian, "the function of the protocol or its availability could be impacted" is not the same a conflicting with specs; function and availability is not specs. In what way do you mean that the protocol stops working?

[elmutt]

I believe this solves it based on original authors recommendation:

asymmetryfinance/afeth#169

also

asymmetryfinance/afeth#193

[c4-sponsor]

elmutt (sponsor) confirmed