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

Last Trove may be prevented from redeeming #381

Open
code423n4 opened this issue Mar 3, 2023 · 6 comments
Open

Last Trove may be prevented from redeeming #381

code423n4 opened this issue Mar 3, 2023 · 6 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 edited-by-warden judge review requested Judge should review this issue M-09 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report

Comments

@code423n4
Copy link
Contributor

code423n4 commented Mar 3, 2023

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/RedemptionHelper.sol#L128

Vulnerability details

Impact

In redeemCollateral() of RedemptionHelper.sol, the LUSD balanceOf the redeemer is checked against the specific collateral recorded LUSD debt (both active and defaulted).

function redeemCollateral(
        address _collateral,
        address _redeemer,
        uint _LUSDamount,
        address _firstRedemptionHint,
        address _upperPartialRedemptionHint,
        address _lowerPartialRedemptionHint,
        uint _partialRedemptionHintNICR,
        uint _maxIterations,
        uint _maxFeePercentage
    )
        external override
    {
        _requireCallerIsTroveManager();
        _requireValidCollateralAddress(_collateral);
        RedemptionTotals memory totals;

        _requireValidMaxFeePercentage(_maxFeePercentage);
        _requireAfterBootstrapPeriod();
        totals.price = priceFeed.fetchPrice(_collateral);
        ICollateralConfig collateralConfigCached = collateralConfig;
        totals.collDecimals = collateralConfigCached.getCollateralDecimals(_collateral);
        totals.collMCR = collateralConfigCached.getCollateralMCR(_collateral);
        _requireTCRoverMCR(_collateral, totals.price, totals.collDecimals, totals.collMCR);
        _requireAmountGreaterThanZero(_LUSDamount);
        _requireLUSDBalanceCoversRedemption(lusdToken, _redeemer, _LUSDamount);

        totals.totalLUSDSupplyAtStart = getEntireSystemDebt(_collateral);
        // Confirm redeemer's balance is less than total LUSD supply
        assert(lusdToken.balanceOf(_redeemer) <= totals.totalLUSDSupplyAtStart);
        ...
}

This makes sense in a single collateral system such as Liquity, but is problematic in a multi-collateral one like Reserve. Since each collateral type tracks its own debt but mints the same LUSD token, LUSD supply (and thus balance) being less than the collateral debt is no longer an invariant. This can can result in:

  • Last trove may be prevented from redeeming by griefers.
  • Users that deposit into multiple Trove types may be prevented from redeeming.

Proof of Concept

Last trove may be prevented from redeeming

Consider the cases when

- There are 2 Trove types (wBTC and wETH). 
- There is 10000 total LUSD debt in the wBTC Troves.
- Stability Pool has 150 LUSD deposited i.e. full liquidity to offset debt.
- There is 100 total LUSD debt in the wETH pool.
- ETH prices crash and all Troves get liquidated except the last one.

A griefer can front-run the last Trove from redeeming by sending the user weth with the amount entireSystemDebt + 1.

In a similar case as above, any users that may borrow from multiple Troves types such that their LUSD balance is greater than the total collateral debt will be prevented from redeeming. However, this is not as problematic because they can just send their excess tokens out.

Tools Used

Manual Review

Recommended Mitigation Steps

  • Consider removing this check as the invariant no longer applies
@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 Mar 3, 2023
code423n4 added a commit that referenced this issue Mar 3, 2023
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Mar 9, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Mar 9, 2023

trust1995 marked the issue as primary issue

@c4-judge
Copy link
Contributor

c4-judge commented Mar 9, 2023

trust1995 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Mar 9, 2023
@tess3rac7
Copy link

dupe #549 leaving up to judge how to handle

@c4-sponsor c4-sponsor added the judge review requested Judge should review this issue label Mar 15, 2023
@c4-sponsor
Copy link

tess3rac7 requested judge review

@c4-judge c4-judge added duplicate-549 and removed primary issue Highest quality submission among a set of duplicates labels Mar 20, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as duplicate of #549

@c4-judge
Copy link
Contributor

trust1995 marked the issue as selected for report

@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Mar 20, 2023
@C4-Staff C4-Staff added the M-09 label Mar 27, 2023
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 edited-by-warden judge review requested Judge should review this issue M-09 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

5 participants