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

Potential zero transfer issue in UStbMinting contract's _transferCollateral function due to small amountToTransfer calculation #9

Closed
c4-bot-4 opened this issue Nov 11, 2024 · 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 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

  1. Incorrect Distribution: Small collateral amounts may result in zero transfers to some addresses in route.addresses, leaving only the last address with the full collateral.

  2. Operational Imbalance: Custody wallets expecting specific collateral shares may receive nothing, creating operational inconsistencies.

  3. Trust and Compliance Risks: Unbalanced collateral routing could lead to trust issues and potential regulatory or contractual concerns, particularly for users relying on predictable distribution.

Proof of Concept

  1. Setup and Conditions:

    • Assume the amount to transfer is smaller than ROUTE_REQUIRED_RATIO (10,000).
    • Ratios are provided to divide the amount among several addresses, but because amount is small, each calculated amountToTransfer will be zero due to integer division.
  2. Example Scenario:

    • amount = 5,000
    • ratios = [1, 1, 1, 9_997] (for 4 addresses)
    • ROUTE_REQUIRED_RATIO = 10,000
    • amountToTransfer for each address:
      amountToTransfer[i] = 5_000 * 1 / 10,000 = 0
    • This results in zero transfer to the first three addresses, with the entire amount going only to the last address.
  3. Demonstration Code:

    uint128 amount = 5000;
    uint128[] memory ratios = new uint128[](4);
    ratios[0] = 1;
    ratios[1] = 1;
    ratios[2] = 1;
    ratios[3] = 9997;
    
    address[] memory addresses = new address[](4);
    addresses[0] = 0xAddress1;
    addresses[1] = 0xAddress2;
    addresses[2] = 0xAddress3;
    addresses[3] = 0xAddress4;
    
    for (uint128 i = 0; i < addresses.length; i++) {
        uint128 amountToTransfer = (amount * ratios[i]) / ROUTE_REQUIRED_RATIO;
        // Each amountToTransfer is 0, except for the last address
        console.log("Amount to transfer to address", i, amountToTransfer);
    }
  4. Expected Outcome:

    • The first three addresses receive zero, and only the fourth address receives 5,000.
    • This demonstrates how integer division causes an imbalance, contradicting the expected proportional distribution.

Recommended Mitigation Steps

Minimum Transfer Check: Ensure amountToTransfer is > 0 before transferring; if zero, redistribute or adjust to ensure all addresses receive a non-zero share.

@c4-bot-4 c4-bot-4 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 Nov 11, 2024
c4-bot-4 added a commit that referenced this issue 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 primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Nov 12, 2024
@iethena
Copy link

iethena commented Nov 15, 2024

It should be noted that only the Collateral Manager can initiate transfers of collateral and it is expected that the collateral manager will preview their transactions to protect against input mistakes. It is also not intended for negligible proportions of collateral to be distributed to various custodians. We therefore argue that the likelihood of this scenario occurring is low. The impact is also low as the worst case scenario is that all of the collateral is distributed to single custodian. There would be an operational overhead Ethena to manually redistribute the collateral in this case directly with the custodian, however no collateral is lost and no user positions are affected. We suggest marking this issue as Low severity.

@iethena iethena added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 15, 2024
@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 18, 2024
@0xEVom
Copy link

0xEVom commented Nov 18, 2024

The COLLATERAL_MANAGER is trusted as per the README.

Besides, this has been accounted for and as the sponsor says the worst case scenario is that all collateral is transferred to the last custodian in the route.

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 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants