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

Inconsistent order_id type in ORDER_TYPE definition and encodeOrder function for EIP-712 #7

Open
c4-bot-6 opened this issue Nov 11, 2024 · 3 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#L32-L35
https://github.com/code-423n4/2024-11-ethena-labs/blob/main/contracts/ustb/UStbMinting.sol#L442-L456

Vulnerability details

Impact

The mismatch between the order_id type (string in ORDER_TYPE and bytes32 in encodeOrder) can cause unexpected behavior during order hashing and EIP712 signature verification. This inconsistency could result in order verification failures, even if the provided signature is correct. It could prevent valid minting and redemption orders from processing, thereby impacting users who depend on this contract’s stablecoin services.

Proof of Concept

In the UStbMinting contract:

  1. The ORDER_TYPE constant on UStbMinting.sol#L32 expects order_id as a string:
    bytes32 private constant ORDER_TYPE = keccak256("Order(string order_id, ...)");
  2. However, in the encodeOrder function on UStbMinting.sol#L442-L456, the order_id is converted to bytes32:
    abi.encode(
        ORDER_TYPE,
        keccak256(bytes(order.order_id)), // This outputs bytes32, while ORDER_TYPE expects string
        ...
    );
  3. This discrepancy can cause the generated EIP712 hash to mismatch, leading to failed signature verification.

In https://eips.ethereum.org/EIPS/eip-712#definition-of-typed-structured-data-%F0%9D%95%8A

Definition: The dynamic types are bytes and string. These are like the atomic types for the purposed of type declaration, but their treatment in encoding is different.

Recommended Mitigation Steps

 abi.encode(
       ORDER_TYPE,
--     keccak256(bytes(order.order_id))
++     keccak256(order.order_id),
       ...
   );
@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-3 c4-bot-3 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
Copy link

0xEVom marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added 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 reopened this Nov 12, 2024
@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 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
@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