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

Precision loss in verifyStablesLimit function may cause incorrect validation for collateral amounts #8

Open
c4-bot-6 opened this issue Nov 11, 2024 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-11-ethena-labs/blob/main/contracts/ustb/UStbMinting.sol#L544-L575

Vulnerability details

Impact

When collateralDecimals > ustbDecimals, the calculation normalizedCollateralAmount = collateralAmount / scale introduces potential precision loss. This may cause the condition differenceInBps <= stablesDeltaLimit to evaluate as true inappropriately when normalizedCollateralAmount <= ustbAmount during redemption. This could lead to inaccurate validations, potentially allowing redemption amounts outside the intended bounds.

Proof of Concept

function test_verify_stables_limit() external {
    vm.prank(benefactor);
    USDTToken.mint(25000 * 10 ** 18);

    uint128 stablesDeltaLimit = 100; // 100 bps

    vm.prank(owner);
    UStbMintingContract.setStablesDeltaLimit(stablesDeltaLimit);

    uint128 ustbAmount = 1000 * 10 ** 6; // 1,000 UStb

    uint128 usdtAmountAtUpperLimit = 1010 * 10 ** 18; // 100 bps above the USDT amount that should be at the upper bps limit
    uint128 usdtAmountAtUpperLimitLg = 1999 * 10 ** 18; // larger than 100 bps above the USDT amount
    uint128 usdtAmountAtLowerLimit = 990 * 10 ** 18; // 100 bps below the USDT amount that should be at the lower bps limit

    address usdtAddress = address(USDTToken);

    assertEq(
        UStbMintingContract.verifyStablesLimit(
            usdtAmountAtUpperLimit,
            ustbAmount,
            usdtAddress,
            IUStbMinting.OrderType.MINT
        ),
        true
    );

    assertEq(
        UStbMintingContract.verifyStablesLimit(
            usdtAmountAtUpperLimitLg,
            ustbAmount,
            usdtAddress,
            IUStbMinting.OrderType.MINT
        ),
        false
    );

    assertEq(
        UStbMintingContract.verifyStablesLimit(
            usdtAmountAtLowerLimit,
            ustbAmount,
            usdtAddress,
            IUStbMinting.OrderType.REDEEM
        ),
        true
    );
}

Explanation

In the above test case, when validating usdtAmountAtLowerLimit, the function returns true even if the normalized value during redemption should have been checked more accurately. This confirms that precision loss can impact validation results.

Recommended Mitigation Steps

Introduce rounding mechanisms or adjust precision handling to avoid precision loss during the division operation:

  • Implement SafeMath-like rounding functions.
  • Consider using higher precision types if needed (e.g., uint256) or introducing safeguards to handle scaling differences more accurately.
@c4-bot-6 c4-bot-6 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 11, 2024
c4-bot-6 added a commit that referenced this issue Nov 11, 2024
@c4-bot-9 c4-bot-9 removed the 3 (High Risk) Assets can be stolen/lost/compromised directly label Nov 11, 2024
@code4rena-admin code4rena-admin added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value edited-by-warden labels Nov 11, 2024
@c4-bot-11 c4-bot-11 added the 🤖_primary AI based primary recommendation label Nov 11, 2024
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 12, 2024
@c4-judge
Copy link

0xEVom marked the issue as satisfactory

@c4-judge
Copy link

0xEVom marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed satisfactory satisfies C4 submission criteria; eligible for awards labels Nov 12, 2024
@c4-judge
Copy link

0xEVom changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 12, 2024
@c4-judge c4-judge reopened this Nov 12, 2024
@c4-judge c4-judge reopened this Nov 12, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Nov 12, 2024
@c4-judge
Copy link

This previously downgraded issue has been upgraded by 0xEVom

1 similar comment
@c4-judge
Copy link

This previously downgraded issue has been upgraded by 0xEVom

@c4-judge c4-judge removed the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 12, 2024
@c4-judge
Copy link

0xEVom removed the grade

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Nov 12, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Nov 12, 2024
@c4-judge
Copy link

0xEVom changed the severity to QA (Quality Assurance)

@iethena iethena added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 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

7 participants