Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inefficient reward compounding in Votium Strategy #44

Open
c4-submissions opened this issue Sep 27, 2023 · 11 comments
Open

Inefficient reward compounding in Votium Strategy #44

c4-submissions opened this issue Sep 27, 2023 · 11 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Sep 27, 2023

Lines of code

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

Vulnerability details

Summary

Rewards acquired through the Votium strategy undergo a sequence of steps that involve multiple intermediate swaps, leading to an inefficient process that can negatively impact the protocol value.

Impact

Rewards obtained from the VotiumStrategy contract are compounded and deposited back into the protocol.

reward

After tokens are claimed, the Rewarder role calls applyRewards() to exchange rewards coming from Votium and Convex to ETH. The resulting amount of ETH is then sent to AfEth, which, depending on the current TVL ratio between the two collaterals, will end up either staking the ETH in SafEth, or depositing the ETH back to the VotiumStrategy contract, which will swap that ETH again to CVX tokens in order to lock them in the Convex platform.

This means that rewards are swapped to ETH, only to be swapped again to other assets. In the case of SafEth, the staked ETH will be distributed according to the current weights of the different underlying staking protocols configured. In the case of VotiumStrategy the ETH will be swapped back to CVX.

It can be argued that it would be too difficult to handle this case efficiently for the SafEth staking scenario: the contracts in AfEth would need to know of the underlying split of the different LSD in order to know how to execute the swap. This could cause too much coupling and would require big architecture changes.

However, for the sole case of VotiumStrategy, a more efficient approach can be adopted. Knowing in advance that the rewards will be compounded back into the VotiumStrategy contract, rewards can be directly swapped to CVX instead of going through ETH first. This change is straightforward to implement, since the current intention is to use the Ox protocol: instead of swapping everything to ETH, just swap it to CVX and lock it.

Recommendation

To implement a more efficient reward compounding process, the current reward flow can be refactored to just swap to ETH when the conditions will make the rewards go into SafEth (safEthRatio < ratio). In the other case (when rewards should be deposited back the Votium strategy), the implementation can directly swap rewards to CVX.

Assessed type

Other

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 27, 2023
c4-submissions added a commit that referenced this issue Sep 27, 2023
@toshiSat
Copy link

This seems more of a gas optimization

@romeroadrian
Copy link

This seems more of a gas optimization

I was referring to the intermediate swap steps used to compound the rewards. In 2 out of 3 scenarios, rewards are first sold for ETH only to be swapped immediately back to CVX. The last leg is even fixed to the CVX/ETH pool. The recommendation here is to go directly with 0x, which would give a more optimized path, earning more value for depositors.

@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2023

0xleastwood marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Oct 3, 2023
@0xleastwood
Copy link

I agree that this is more of a optimisation that doesn't necessarily leak value unless the allowed slippage for performing two swaps is greater than if we were to route the swaps differently.

@0xleastwood
Copy link

To me, it makes sense to downgrade this to QA because it isn't clear how this might be exploited.

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Oct 4, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2023

0xleastwood changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax grade-a labels Oct 4, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2023

0xleastwood marked the issue as grade-a

@c4-sponsor
Copy link

elmutt (sponsor) confirmed

@c4-sponsor c4-sponsor added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Oct 4, 2023
@c4-sponsor
Copy link

elmutt (sponsor) acknowledged

@c4-sponsor
Copy link

elmutt (sponsor) confirmed

@c4-sponsor c4-sponsor added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Oct 4, 2023
@c4-judge c4-judge removed the grade-a label Oct 5, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Oct 5, 2023

0xleastwood removed the grade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

8 participants