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

[Bug] sol! macro doesn't alphabetize nested struct postfixes in SolStruct::eip712_encode_type() #201

Closed
nhtyy opened this issue Jul 20, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@nhtyy
Copy link

nhtyy commented Jul 20, 2023

Component

sol-type

What version of Alloy are you on?

├── alloy-sol-types v0.2.0 │ ├── alloy-primitives v0.2.0 │ ├── alloy-sol-macro v0.2.0 (proc-macro)

Operating System

macOS (Apple Silicon)

Describe the bug

according to eip712

"If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding. An example encoding is Transaction(Person from,Person to,Asset tx)Asset(address token,uint256 amount)Person(address wallet,string name)."

use alloy_sol_types::sol;

sol! {
    /// @dev generic order information
    ///  should be included as the first field in any concrete order types
    struct OrderInfo {
        // The address of the reactor that this order is targeting
        // Note that this must be included in every order so the swapper
        // signature commits to the specific reactor that they trust to fill their order properly
        address reactor;
        // The address of the user which created the order
        // Note that this must be included so that order hashes are unique by swapper
        address swapper;
        // The nonce of the order, allowing for signature replay protection and cancellation
        uint256 nonce;
        // The timestamp after which this order is no longer valid
        uint256 deadline;
        // Custom validation contract
        address additionalValidationContract;
        // Encoded validation params for additionalValidationContract
        bytes additionalValidationData;
    }

    /// @dev An amount of output tokens that decreases linearly over time
    struct DutchOutput {
        // The ERC20 token address (or native ETH address)
        address token;
        // The amount of tokens at the start of the time period
        uint256 startAmount;
        // The amount of tokens at the end of the time period
        uint256 endAmount;
        // The address who must receive the tokens to satisfy the order
        address recipient;
    }

    /// @dev An amount of input tokens that increases linearly over time
    struct DutchInput {
        // The ERC20 token address
        address token;
        // The amount of tokens at the start of the time period
        uint256 startAmount;
        // The amount of tokens at the end of the time period
        uint256 endAmount;
    }

    struct ExclusiveDutchOrder {
        // generic order information
        OrderInfo info;
        // The time at which the DutchOutputs start decaying
        uint256 decayStartTime;
        // The time at which price becomes static
        uint256 decayEndTime;
        // The address who has exclusive rights to the order until decayStartTime
        address exclusiveFiller;
        // The amount in bps that a non-exclusive filler needs to improve the outputs by to be able to fill the order
        uint256 exclusivityOverrideBps;
        // The tokens that the swapper will provide when settling the order
        DutchInput input;
        // The tokens that must be received to satisfy the order
        DutchOutput[] outputs;
    }
}

using cargo expand we see that

fn eip712_encode_type() -> Cow<'static, str> {
{
    let mut encoded = String::from(
        "ExclusiveDutchOrder(OrderInfo info,uint256 decayStartTime,uint256 decayEndTime,address exclusiveFiller,uint256 exclusivityOverrideBps,DutchInput input,DutchOutput[] outputs)",
    );
    if let Some(s)
        = <OrderInfo as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <::alloy_sol_types::sol_data::Uint<
            256,
        > as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <::alloy_sol_types::sol_data::Uint<
            256,
        > as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <::alloy_sol_types::sol_data::Address as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <::alloy_sol_types::sol_data::Uint<
            256,
        > as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <DutchInput as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    if let Some(s)
        = <::alloy_sol_types::sol_data::Array<
            DutchOutput,
        > as ::alloy_sol_types::SolType>::eip712_encode_type() {
        encoded.push_str(&s);
    }
    encoded
}
    .into()
}

So in words, an OrderInfo may be appended before DutchInput or DutchOutput which would violate eip712 rules

@nhtyy nhtyy added the bug Something isn't working label Jul 20, 2023
@nhtyy
Copy link
Author

nhtyy commented Jul 20, 2023

also some other things being pushed, not really sure what thats about, heres the expansion for OrderInfo which in comparison doesnt have any nested structs

fn eip712_encode_type() -> Cow<'static, str> {
  "OrderInfo(address reactor,address swapper,uint256 nonce,uint256 deadline,address additionalValidationContract,bytes additionalValidationData)"
      .into()
}

@DaniPopes
Copy link
Member

Fixed in #203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants