Inefficient reward split could cause unbalanced ratio and favor SafEth staking

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

Reward splitting is executed in a greedy fashion, potentially causing a permanent unbalanced state which deviates from the desired ratio.

Impact

Collaterals in the AfEth protocol are balanced according to a ratio. New deposits in the contract are split between SafEth and VotiumStrategy according to this ratio. Compounded rewards coming from the Votium strategy are also affected by this ratio, this can be seen in the implementation of depositRewards():

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:     }

Both SafEth and Votium TVLs are calculated in order to normalize and determine the proportion between the two. This result is compared to the configured ratio in order to know where to route rewards.

As we can see in the previous snippet of code, rewards go either to SafEth or to Votium. This means that rewards are allocated in a greedy fashion: if the SafEth ratio is below the threshold, then everything gets staked in SafEth. Else, everything goes to Votium.

This can derive in a permanent state of unbalanced weights that fails to converge to the desired ratio. If the safEthRatio is slightly below the threshold, then all goes there. Similarly, if the safEthRatio is slightly above the ratio, then everything goes to Votium. This could create an scenario in which the resulting ratio bounces off the desired value.

Recommendation

The issue can be fixed by considering a split of the rewards between both collaterals so that the ratio is respected (or try to minimize the offset, if the reward is not enough).

If the offset is too large in the direction of SafEth, then all rewards are staked in SafEth. If the offset is too large in the direction of Votium, then all rewards are deposited in VotiumStrategy. Here, too large means that adding the whole set of rewards into a single collateral it doesn't offset the ratio in the other direction (e.g. for SafEth this would be (T_s + R_s) / (T + R) < ratio) . In any other case, the rewards should be proportionally distributed so that the ratio is kept (i.e. (T_s + R_s) / (T + R) ~= ratio).

Say T is the total TVL, Ts is the TVL of SafEth, Tv is the TVL of Votium, R are the total new rewards, Rs the rewards that should go to SafEth, and Rv the rewards that should go to Votium.

T = Tv + Ts
R = Rv + Rs

(Ts + Rs) / (Ts + Tv + Rs + Rv) = (Ts + Rs) / (T + R) = ratio

Assessed type

Other

[elmutt]

We consider this an acceptable edge case

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[elmutt]

I previously said this is acceptable but its probably worth fixing if we have time

[c4-sponsor]

elmutt (sponsor) confirmed

[d3e4]

This does not meet the criteria for a H/M issue. There is no real impact. The report demonstrates no harm in having the balances slightly off ratio (what is the ideal ratio anyway?). And the rewards are small in comparison, so the overshoots will be insignificant. The current implementation is clearly a design choice, meant to reduce overhead in exchange for a noisy fixpoint.

[romeroadrian]

This does not meet the criteria for a H/M issue. There is no real impact. The report demonstrates no harm in having the balances slightly off ratio (what is the ideal ratio anyway?). And the rewards are small in comparison, so the overshoots will be insignificant. The current implementation is clearly a design choice, meant to reduce overhead in exchange for a noisy fixpoint.

The impact is a deviation of the desired ratio, which is a protocol feature.

[d3e4]

The impact is a deviation of the desired ratio, which is a protocol feature.

Where is it stated that the ratio must be adhered to with no error? Clearly the devs knew that their implementation would result in some overshoot. Unless you can demonstrate that the overshoot is greater than expected or has some unintended consequences, you haven't shown anything new.

[romeroadrian]

Where is it stated that the ratio must be adhered to with no error?

Really, d?

Clearly the devs knew that their implementation would result in some overshoot.

??? Do you mean that the devs were aware of the potential issues before requesting the audit?

[0xleastwood]

Siding with @romeroadrian on this, the fact that there is no spec to confirm abnormal behaviour does not invalidate the finding. The sponsor has acknowledged the issue and wishes to fix it if the time suffices. To me, it is clear they were firstly not aware of it in the first place and find the impact significant enough that it requires fixing.

[d3e4]

I agree there is nothing invalid in this report. Only, there is no security impact. The sponsor has acknowledged a design suggestion. This might, or might not, make the protocol better; this is for the sponsor to decide. But, as far as this report can tell, whether this change is implemented or not, the protocol is just as secure. Security is H/M-issues, broader design considerations is Analysis.

[0xleastwood]

Noted, I didn't know how analyses would fit into the context of other findings but I do think this is high-level advice to improve an existing design but it does not necessarily highlight a security issue. I'm finding it hard to make this satisfy the criteria for medium severity but am open to more info @romeroadrian.

[0xleastwood]

Downgrading to QA for the time-being.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

I agree that splitting rewards in a greedy fashion is not an ideal way to deal with things but rewards are accumulating slowly and hence this imbalance will most likely be negligible. Keeping this as QA.