Direct depositors in the Votium strategy will lose rewards when these are routed to SafEth

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

The VotiumStrategy contract can be used directly without interfacing with AfEth. However, rewards coming from deposits can be eventually routed to SafEth when passing through the manager (AfEth). In this scenario, users that deposit directly in VotiumStrategy will lose their rewards.

Impact

The VotiumStrategy contract is partially agnostic to the AfEth contract. Accounts can freely deposit and withdraw directly in the VotiumStrategy contract, but rewards can be potentially compounded through the manager contract (which is AfEth).

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

272:     function applyRewards(SwapData[] calldata _swapsData) public onlyRewarder {
...
302:         if (address(manager) != address(0))
303:             IAfEth(manager).depositRewards{value: ethReceived}(ethReceived);
304:         else depositRewards(ethReceived);

If the manager is present, rewards will be delegated to AfEth::depositRewards().

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

288:         if (safEthRatio < ratio) {
289:             ISafEth(SAF_ETH_ADDRESS).stake{value: amount}(0);
290:         } else {
291:             votiumStrategy.depositRewards{value: amount}(amount);
292:         }

Depending on the current TVL ratio, it is possible that these rewards are fully routed to the SafEth contract and staked there. If this is the case, then direct depositors of the VotiumStrategy contract will lose their rewards, since these are not compounded in VotiumStrategy, but in SafEth.

Proof of Concept

1. A user deposits directly in VotiumStrategy and holds vAfEth tokens.
2. Rewards in VotiumStrategy are claimed and applied by the rewarded role.
3. applyRewards() calls IAfEth::depositRewards() with the ETH amount coming from rewards.
4. In AfEth, depositRewards() calculates that the current safEthRatio is below the defined ratio and calls ISafEth::stake(), forwarding the ETH amount.
5. Rewards are staked in SafEth. User gets nothing.

Recommendation

There are two possible paths here. One is to forbid direct interaction with VotiumStrategy and only allow the manager to deposit and withdraw from it.

The alternative is to account for deposits that are directly made to the VotiumStrategy (i.e. where the caller is not the manager) and, when rewards are applied, split the reward amount based on the ratio of deposits made by AfEth and deposits made by other accounts. The part that belongs to AfEth can be routed through AfEth::depositRewards() and the part that belongs to direct depositors should be handled directly by VotiumStrategyCore::depositRewards().

Assessed type

Other

[elmutt]

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

we went with locking it down so only afEth deposit or withdraw from votium strategy

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

Don't expect these functions to be exposed by the front-end, users would have to intentionally call these functions which is not typical contract use. Downgrading to medium severity as it is assumed that users are incorrectly interacting with the contract in the first place.

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[d3e4]

VotiumStrategy doesn't promise any rewards to users, so they should not get any. This is even mentioned in the README: "people could directly deposit to votium, the problem would be their rewards get spread into the manager". Depositing directly into VotiumStrategy is a user mistake (which they can even correct by withdrawing again). They should not be owed any rewards for their mistake.

[romeroadrian]

Depositing directly into VotiumStrategy is a user mistake (which they can even correct by withdrawing again). They should not be owed any rewards for their mistake.

Hey, this is not true. Even if technically feasible, the docs state this is valid behavior.

[0xleastwood]

I'm a bit confused, can you explain where it is stated as valid behaviour?

[0xleastwood]

It sounds like it is expected behaviour if users deposit directly to the votium contract, then they are not owed rewards and it is instead spread to everyone else?

[romeroadrian]

It sounds like it is expected behaviour if users deposit directly to the votium contract, then they are not owed rewards and it is instead spread to everyone else?

Sorry, by expected behavior I meant depositing directly in the Votium strategy, in reply to d3 saying that this is user mistake.

[0xleastwood]

Going to side with @d3e4 on this, it seems this is stated in the documentation and even though depositing directly isn't explicitly denied, there is no reason as to why users would do this and intentionally lose out on potential rewards.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as unsatisfactory: Invalid

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)