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

A new era might be triggered despite a significant value being held in the previous era #2

Open
code423n4 opened this issue Jun 7, 2023 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working H-02 rainout Used to specify findings that came in during the rained-out audit satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L441-L444
https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L457-L460

Vulnerability details

When RSR seizure occurs the staking and drafting rate is adjusted accordingly, if any of those rates is above some threshold then a new era begins (draft or staking era accordingly), wiping out all of the holdings of the current era.
The assumption is that if the rate is above the threshold then there's not much staking or drafts left after the seizure (and therefore it makes sense to begin a new era).
However, there might be a case where a previous seizure has increased the staking/draft rate close to the threshold, and then even a small seizure would make it cross this threshold. In that case the total value of staking or drafts can be very high, and they will all be wiped out by starting a new era.

Impact

Stakers will lose their holdings or pending drafts.

Proof of Concept

Consider the following scenario:

  • Max stake rate is 1e9
  • A seizure occurs and the new rate is now 91e7
  • Not much staking is left after the seizure, but as time passes users keep staking bring back the total stakes to a significant value
  • A 10% seizure occurs, this causes the staking rate to cross the threshold (getting to 1.01e9) and start a new era

This means the stakings were wiped out despite holding a significant amount of value, causing a loss for the holders.

Recommended Mitigation Steps

This one is a bit difficult to mitigate.
One way I can think of is to add a 'migration' feature, where in such cases a new era would be created but users would be able to transfer the funds that they held in the previous era into the new era. But this would require some significant code changes and checking that this doesn't break anything or introduces new bugs.

Assessed type

Other

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 7, 2023
code423n4 added a commit that referenced this issue Jun 7, 2023
@tbrent
Copy link

tbrent commented Jun 12, 2023

@0xA5DF thoughts on a governance function that requires the ratio be out of bounds, that does beginEra() and/or beginDraftEra()?

The idea is that stakers can mostly withdraw, and since governance thresholds are all percentage, vote to immolate themselves and re-start the staking pool. I think it should treat beginEra() and beginDraftEra() separately, but I'm not confident in that yet.

@itsmetechjay itsmetechjay added the rainout Used to specify findings that came in during the rained-out audit label Jun 14, 2023
@tbrent
Copy link

tbrent commented Jul 4, 2023

We're still not sure how to mitigate this one. Agree it should be considered HIGH and a new issue.

@c4-sponsor c4-sponsor added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Jul 4, 2023
@c4-sponsor
Copy link

tbrent marked the issue as sponsor acknowledged

@c4-judge
Copy link

0xean marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jul 12, 2023
@C4-Staff C4-Staff added selected for report This submission will be included/highlighted in the audit report H-02 labels Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working H-02 rainout Used to specify findings that came in during the rained-out audit satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

6 participants