Skip to content

setEpochDuration() breaks the user's expected exchangeRate #83

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

Closed
code423n4 opened this issue Jan 15, 2023 · 5 comments
Closed

setEpochDuration() breaks the user's expected exchangeRate #83

code423n4 opened this issue Jan 15, 2023 · 5 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 duplicate-205 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L546-L552

Vulnerability details

Impact

Extended epoch time, resulting in exchangeRate changes that affect user revenue

Proof of Concept

MANAGER_ADMIN can modify epochDuration with setEpochDuration()
epochDuration is an important factor in the calculation of whether to enter a new epoch

However, setEpochDuration() does not first perform an updateEpoch (if it needs to be currently satisfied to enter a new epoch, it enters a new epoch)
As a result, it may not be possible to switch to the new epoch because the epochDuration has grown, and the old epoch is still used.

As each epoch will have its own exchangeRate, the user has already estimated the exchangeRate before the end of the epoch, so the requestMint() is performed.
Since setEpochDuration changed the epochDuration, the exchangeRate changed. when exchangeRate changes ,maybe affect user revenue

So it is recommended to calculate the old epochDuration for the elapsed time to get the correct exchangeRate

Tools Used

Recommended Mitigation Steps

add updateEpoch

  function setEpochDuration(
    uint256 _epochDuration
- ) external onlyRole(MANAGER_ADMIN) {
+ ) external updateEpoch onlyRole(MANAGER_ADMIN) {
    uint256 oldEpochDuration = epochDuration;
    epochDuration = _epochDuration;
    emit EpochDurationSet(oldEpochDuration, _epochDuration);
  }
@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 labels Jan 15, 2023
code423n4 added a commit that referenced this issue Jan 15, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as primary issue

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 22, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@ypatil12
Copy link

ypatil12 commented Jan 31, 2023

Dup Fixed here: https://github.com/ondoprotocol/flux/issues/167

@c4-sponsor
Copy link

ali2251 marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jan 31, 2023
@C4-Staff C4-Staff closed this as completed Feb 3, 2023
@C4-Staff
Copy link

C4-Staff commented Feb 3, 2023

JeeberC4 marked issue #205 as primary and marked this issue as a duplicate of 205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 duplicate-205 satisfactory satisfies C4 submission criteria; eligible for awards 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

5 participants