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.

Sign up for GitHub

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

Jump to bottom

Furnace would melt less than intended #37

Open
code423n4 opened this issue Aug 22, 2023 · 4 comments
Open

Furnace would melt less than intended #37

code423n4 opened this issue Aug 22, 2023 · 4 comments
Labels
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 MR-M-04 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/99d9db72e04db29f8e80e50a78b16a0b475d79f3/contracts/p1/Furnace.sol#L92-L105

Vulnerability details

We traded one problem with another here
The original issue was that in case melt() fails then the distribution would use the new rate for previous periods as well.
The issue now is that in case of a failure (e.g. paused or frozen) we simply don’t melt for the previous period. Meaning RToken holders would get deprived of the melting they’re supposed to get.

This is esp. noticeable when the ratio has been decreased and the balance didn’t grow much, in that case we do more harm than good by updating lastPayout and lastPayoutBal.

A better mitigation might be to update the lastPayout in a way that would reflect the melting that should be distributed.

Assessed type

Other
@code423n4 code423n4 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 MR-M-04 labels Aug 22, 2023
code423n4 added a commit that referenced this issue Aug 22, 2023
@code423n4
0xA5DF issue #37
f183330
@tbrent
Copy link

tbrent commented Aug 24, 2023

I think this can only happen when frozen, not while paused. Furnace.melt() and RToken.melt() succeed while paused.

@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 Aug 24, 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 Aug 28, 2023
@c4-judge
Copy link

c4-judge commented Sep 6, 2023

0xean marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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 MR-M-04 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
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tbrent @code423n4 @c4-judge @c4-sponsor