AfEth collaterals cannot be balanced after ratio is changed

[c4-submissions]

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

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[elmutt]

Its hard to rebalance because of the locked convex. We have discussed this internally and consider it an acceptable risk for now so im acknowledging this issue instead of confirming

[c4-sponsor]

elmutt marked the issue as disagree with severity

[c4-sponsor]

elmutt (sponsor) acknowledged

[c4-judge]

0xleastwood marked the issue as not selected for report

[0xleastwood]

Leaving as is because I don't think this is typical behaviour.

[c4-judge]

0xleastwood marked the issue as selected for report

[Rassska]

If the Asymmetry decides to reset the ratio = 5e17 from ratio == 7e17, they would probably set it to some small acceptable percentage, like 1e17 and then, when the tvl come close to the desired ratio, the reset to ratio = 5e17 should happen. This is just to speed up the rebalancing.

[romeroadrian]

If the Asymmetry decides to reset the ratio = 5e17 from ratio == 7e17, they would probably set it to some small acceptable percentage, like 1e17 and then, when the tvl come close to the desired ratio, the reset to ratio = 5e17 should happen. This is just to speed up the rebalancing.

I don't think the CVX locking constraint takes away the issue. Note that the same is implemented in SafEth, with derivatives having different constraints in their respective LSD token.

For the specific concern in the Votium strategy and the CVX locking, it could be implemented by increasing the cvxUnlockObligations. Note that AfEth contract is a client of VotiumStrategy after all!

[0xleastwood]

I mean you could implement instant rebalancing but that probably adds too much complexity and isn't necessary, so instead this design choice was decided as acceptable. Don't think it invalidates the issue.

[d3e4]

What is the actual impact of this? A forced rebalance would be associated with trading losses, so letting it rebalance organically seems like a good design choice. What is the harm in staying in the old ratio for a while? That was the exposure that was first deposited into after all, so it seems fair that when they then withdraw that they withdraw in about the same ratio. Rather it seems that the ratio can be seen as an agreement with the user on how his funds are to be invested. Suddenly rebalancing these would break this agreement.

As for how fast it converges to the new ratio, if the small amounts are deposited and withdrawn equally, the higher balance will converge as (startBalance - targetBalance) * exp(-x) + targetBalance, where x is the total amount both withdrawn and deposited so far. (In the report's example this is 0.2 * exp(-x) + 0.5). So to reduce the ratio error to a tenth (to turn (30%, 70%) into (48%, 52%)) then ln(10) ≈ 2.30 times the balances must turned over. This does not seem necessarily inappropriately slow.

I talked about this "issue" in my analysis #71.

[romeroadrian]

organically seems like a good design choice

Same as #48, the ratio is a protocol feature.

I strongly disagree with "so letting it rebalance organically seems like a good design choice". Even if we discard situations where the shift in balance is a strategic choice (which I still don't agree with a organic rebalance), a critical scenario in which the exposure to any of the strategies must be reduced will clearly cause an issue.

[d3e4]

organically seems like a good design choice

Same as #48, the ratio is a protocol feature.

I strongly disagree with "so letting it rebalance organically seems like a good design choice". Even if we discard situations where the shift in balance is a strategic choice (which I still don't agree with a organic rebalance), a critical scenario in which the exposure to any of the strategies must be reduced will clearly cause an issue.

The report doesn't demonstrate any such critical scenario, so then there is insufficient evidence. (I also cannot imagine what such a scenario would even be.)

As for the ratio's being a protocol feature, see my comment on #48.

[romeroadrian]

organically seems like a good design choice

Same as #48, the ratio is a protocol feature.
I strongly disagree with "so letting it rebalance organically seems like a good design choice". Even if we discard situations where the shift in balance is a strategic choice (which I still don't agree with a organic rebalance), a critical scenario in which the exposure to any of the strategies must be reduced will clearly cause an issue.

The report doesn't demonstrate any such critical scenario, so then there is insufficient evidence. (I also cannot imagine what such a scenario would even be.)

As for the ratio's being a protocol feature, see my comment on #48.

I don't know what to say, honestly. First it was the lack of impact, then the organic rebalance that was a design choice, now it's a lack of evidence. 🤷

[0xleastwood]

I agree that organic rebalancing is a good feature but this can be easily circumvented by depositing into the protocol under the new ratio, withdrawing and rinsing and repeating to bring it closer to the new ratio at an accelerated rate. But I also understand that instantly rebalancing to the new ratio would retroactively impact existing users which again is not ideal. I don't think either approach is valid and I think the design can either be reconsidered or considered an acceptable risk by the protocol team for which they have. Keeping this as is because imo it is still highlighting an important design flaw.

[d3e4]

I agree that organic rebalancing is a good feature but this can be easily circumvented by depositing into the protocol under the new ratio, withdrawing and rinsing and repeating to bring it closer to the new ratio at an accelerated rate. But I also understand that instantly rebalancing to the new ratio would retroactively impact existing users which again is not ideal. I don't think either approach is valid and I think the design can either be reconsidered or considered an acceptable risk by the protocol team for which they have. Keeping this as is because imo it is still highlighting an important design flaw.

From this I gather that both organic rebalancing and instant rebalancing have drawbacks, and that the current design can either be reconsidered or accepted. So this report achieves nothing but "highlighting an important design flaw", which sounds exactly like Analysis to me, with no security risks demonstrated.

In any case, I would like to remind of the duplicate of this issue in my Analysis #71 under Ratio mechanism.

[romeroadrian]

I agree that organic rebalancing is a good feature but this can be easily circumvented by depositing into the protocol under the new ratio, withdrawing and rinsing and repeating to bring it closer to the new ratio at an accelerated rate. But I also understand that instantly rebalancing to the new ratio would retroactively impact existing users which again is not ideal. I don't think either approach is valid and I think the design can either be reconsidered or considered an acceptable risk by the protocol team for which they have. Keeping this as is because imo it is still highlighting an important design flaw.

From this I gather that both organic rebalancing and instant rebalancing have drawbacks, and that the current design can either be reconsidered or accepted. So this report achieves nothing but "highlighting an important design flaw", which sounds exactly like Analysis to me, with no security risks demonstrated.

In any case, I would like to remind of the duplicate of this issue in my Analysis #71 under Ratio mechanism.

Ok so you have changed your mind again, now it's not a lack of evidence anymore but a lack of impact, and even so you are asking for grading based on part of your analysis. Unbelievable.

[d3e4]

No, they have always gone hand in hand @adriro: there should be evidence for an impact.

[0xleastwood]

Moving into analysis report for the same reasons as #48.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[romeroadrian]

@0xleastwood Sorry, but I have to say that I feel the shift in decision is a product of the constant pressure of a warden that keeps changing his arguments because he doesn't like the outcome. Nevertheless, I'll provide more information and try to address every point.

First, I consider both #55 and #48 issues that have nothing to do with C4 analysis. Citing https://docs.code4rena.com/awarding/incentive-model-and-awards#analyses

Each warden is encouraged to submit an Analysis alongside their findings for each audit, to share high-level advice and insights from their review of the code.

Comments in d3e4 analysis should be taken as an insight of the protocol/design, which doesn't imply that my reported issues have the same intention, nor that they belong to that category. I think this is another rhetoric movement by this person to downgrade the issue or eventually gain grade from it as a duplicate, shamelessly stated here #55 (comment)

Second, regarding the issues itself, I believe both are valid and have the proper severity. The ratio is a core feature of the contract, it measures the exposure to the underlying assets, which ultimately affects depositors. I believe both reports are well structured, providing both impact and evidence.

I think that, between the two, #55 is more important and has a greater latent impact. Any change to the ratio cannot be implemented in practice. This becomes more important in a critical scenario in which the protocol needs to move away from any of the underlying strategies, a change in the ratio will do exactly nothing.

Regarding the instantly rebalancing, this is precisely what SafEth implements, and what my report proposes as a recommendation. It doesn't need to be enforced during every ratio change, but would allow correction with both proper access control given to protocol government, and also proper slippage control to minimize impact (again both concerns highlighted in the report). Note that SafEth compounds itself internally, and only Votium rewards are compounded into AfEth, but these rewards only represent a 20% APR of just the Votium TVL. And this is without considering a shift in price of ETH/CVX, which needs to be factored in since one strategy is essentially ETH/LSD and the other is CVX.

[romeroadrian]

It's also suspiciously curious that the same warden mentions, in this comment, that:

This does not seem necessarily inappropriately slow.

But in his own issue, which interestingly is also based on the unbalanced ratio, says that:

Regarding the case where the underlying is reconverging after a change of ratio it is worth noting that convergence is quite slow. Several times the entire balances must be traded before the new ratio is approached.

It appears that the facts differ when he discusses his own issues compared to when he attempts to criticize others.

[d3e4]

Let me first address the personal attacks.

a warden that keeps changing his arguments because he doesn't like the outcome.

I have already addressed the error in what you have explicitly stated are changes to my arguments. However else you think my comments uses different arguments are due to their being individually incomplete, inadequately expressed, or simply due to the fact the they each are written in response to a specific comment.
In any case, the only thing that matters is the validity of my arguments. Any other criticism of my arguments is, itself, rhetoric.

It's also suspiciously curious that the same warden mentions, in this comment, that:

This does not seem necessarily inappropriately slow.

But in his own issue, which interestingly is also based on the unbalanced ratio, says that:

Regarding the case where the underlying is reconverging after a change of ratio it is worth noting that convergence is quite slow. Several times the entire balances must be traded before the new ratio is approached.

It appears that the facts differ when he discusses his own issues compared to when he attempts to criticize others.

"Not necessarily inappropriately slow" does not contradict "quite slow". I never claimed it is too slow, in the sense that it has an impact.
In my issue that you refer to, the persistence of the imbalance does indeed play a role in an impact. But that is not the core of the issue, and it is not an impact that you mentioned here.

On to the issue itself.
I can see what you mean by the ratio as a core feature. I agree that it is. But it is your own invention that being able to immediately rebalance is a core feature.
Let me summarise your report. Your evidence is that the only way to rebalance is by letting it converge from deposits and withdrawals. (There is also the adjustment from the rewards deposit, which you don't fully mention but would probably agree with me that the effect is likely insufficient.) So the core feature is fulfilled; it is possible to rebalance to a new ratio. They chose this mechanism for rebalancing and it works. The question is then only whether it works well enough, that is, whether it doesn't take an unreasonably long time. You simply state that it will "take a lot of time and may be impractical for most cases", without justification. In my Analysis I at least estimated the speed as "Several times the balances have to be deposited and withdrawn in order to reach close to the new ratio." In my comment above I quantified my estimate and applied it to your example, showing that it doesn't actually take quite that long to converge.
So if "take a lot of time" is an impact, then it is arguably not true, nor have you provided any evidence for it. The issue boils down to the claim that "there is no direct way to balance the assets to follow the new ratio" because there is no direct way to balance the assets to follow the new ratio (!).
Had you been right, that it takes a clearly unacceptably long time to rebalance, then your report would still lack evidence, but it would at least have reported an actual issue.

[romeroadrian]

Let me first address the personal attacks.

Not at all, quite the opposite. I'm raising, with links, cites and evidence, a clear inconsistent attitude that I consider disrespectful towards the work I do.

[0xleastwood]

I will make my final decision on this shortly, but I would like to point out that the post-judging QA is over and my decision will be final.

[0xleastwood]

I'm going to reiterate my understanding of the analysis section, I actually don't think any information which is worthy of being included in this section automatically invalidates any related issues. It makes sense to me that there would be overlap. Ideally architectural/design decisions are highlighted but the impact of those decisions can be brought to light in the form of formal issues too.

In this instance, the implementation of the ratio mechanism with respect to rebalancing between vEth and safEth is a design decision that has flaws and potentially needs to be redesigned. However, the warden has identified that the current implementation does not enforce the ratio as what might be expected from users. For example, once the ratio has been changed, it takes some time for this to be fully in effect and as a result, users may be depositing into the protocol with the expectation of some ratio but that isn't what is currently in place. Performing instant rebalances is a design decision done by safEth so maybe this is the right way to be consistent? Even if users are potentially retroactively affected, provided the delay is sufficient, users should be able to exit the protocol if they do not agree with the new ratio.

For these reasons, I would still like to keep this issue as medium severity because I think this mechanism is ultimately flawed and can be realistically improved. My decision is final on this.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as selected for report

[d3e4]

@0xleastwood I understand your decision is final, and I can agree with your reasoning. But you are missing an important point.
That the rebalancing takes some time is NOT in itself an impact. This is by deliberate design.
What might be an appreciable impact is that it takes too long to rebalance. But this report makes no case for neither how long it takes, nor that the time it takes has any impact. It only states the obvious: that it takes some time.

But since you do grade this issue as Medium, then my report on this issue in #71, which reports all of the facts provided here (and a little more), should be upgraded to a duplicate of this issue.

[0xleastwood]

@0xleastwood I understand your decision is final, and I can agree with your reasoning. But you are missing an important point. That the rebalancing takes some time is NOT in itself an impact. This is by deliberate design. What might be an appreciable impact is that it takes too long to rebalance. But this report makes no case for neither how long it takes, nor that the time it takes has any impact. It only states the obvious: that it takes some time.

But since you do grade this issue as Medium, then my report on this issue in #71, which reports all of the facts provided here (and a little more), should be upgraded to a duplicate of this issue.

Noted, but my decision remains unchanged and I will not be upgrading a section of your analysis report because it doesn't dive into the impact nor does it provide any remediation to the issue at hand.

[d3e4]

@0xleastwood I understand your decision is final, and I can agree with your reasoning. But you are missing an important point. That the rebalancing takes some time is NOT in itself an impact. This is by deliberate design. What might be an appreciable impact is that it takes too long to rebalance. But this report makes no case for neither how long it takes, nor that the time it takes has any impact. It only states the obvious: that it takes some time.
But since you do grade this issue as Medium, then my report on this issue in #71, which reports all of the facts provided here (and a little more), should be upgraded to a duplicate of this issue.

Noted, but my decision remains unchanged and I will not be upgrading a section of your analysis report because it doesn't dive into the impact nor does it provide any remediation to the issue at hand.

It is not a requirement for validity that the report contain a recommended mitigation, it is only encouraged.

@0xleastwood, could you please clarify what you consider to be the stated impact in #55 and what part of that impact is not mentioned in #71.