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] Certain dynamic data fails to decode #167

Closed
Philogy opened this issue Jul 2, 2023 · 6 comments · Fixed by #168
Closed

[Bug] Certain dynamic data fails to decode #167

Philogy opened this issue Jul 2, 2023 · 6 comments · Fixed by #168
Labels
bug Something isn't working

Comments

@Philogy
Copy link

Philogy commented Jul 2, 2023

Component

dyn-abi

What version of Alloy are you on?

├── alloy-dyn-abi v0.2.0 (https://github.com/alloy-rs/core?branch=main#3eab410e) │ ├── alloy-primitives v0.2.0 (https://github.com/alloy-rs/core?branch=main#3eab410e) │ ├── alloy-sol-types v0.2.0 (https://github.com/alloy-rs/core?branch=main#3eab410e) │ │ ├── alloy-primitives v0.2.0 (https://github.com/alloy-rs/core?branch=main#3eab410e) (*) │ │ ├── alloy-sol-macro v0.2.0 (proc-macro) (https://github.com/alloy-rs/core?branch=main#3eab410e) │ │ │ ├── syn-solidity v0.2.0 (https://github.com/alloy-rs/core?branch=main#3eab410e)

Operating System

Linux

Describe the bug

There are certain types of data that the dyn-abi decoder methods (decode_params, decode_sequence, decode_single) fail to decode correctly.

Example 1

Type: (string[],string[],string,uint24,address)

Data (as json):

[
  [
    "gNEwPzb\n",
    "d+`}C{$_",
    "Ji!SFg^f",
    "T{\\+{IU2",
    "Jk~jDY1\n",
    "\n~,WQf;o",
    "Ue::oRI#",
    "{ ']J.jJ"
  ],
  [
    "C*+zbWul",
    "oO,`t680",
    "V(];yDxR"
  ],
  "qYTCBn(E",
  13621740,
  "0x4f5dbfa6851957eab62570ec5a7a7304a7618c0d"
]

Error (from DynSolType::decode_params): Type check failed for "Solidity pointer (uint32)" with data: 674e4577507a620a000000000000000000000000000000000000000000000000

Example 2

Type: (bytes30,bytes31,address,bytes31,bytes32[2],uint112,bytes32,string[])

Data (as json):

[
  "0x9839ca7297d5df5a40d73482859778c00463365089ee823854d274010e93",
  "0xf7f77b3ea9176a6aeb0847c33716cae6567ea7b306405d0b9eb42e78f4ceea",
  "0x804e7f8acf6a8959d46265f779488aaff7f4ef2f",
  "0x6dcb308ae9e6360513d3673f12658cd44be1feec7b8b85a64efabd208587e8",
  [
    "0xea382a6123efaf1fc5c25a99ace015095f4050078b79dbffffd2c286fb672082",
    "0x40a20913c82de7725858dc1cc53c4b10c3561d072e58b4ed5f3f695e26d79324"
  ],
  1613240693945980444269411056144978,
  "0x27d627b4de4b75b83c6bbc12c633efc31593c4b10de2a2a4f77422e42e6fbe47",
  [
    "\r*&k4}.^pOR^x\n25O|\u000b#As#0LGqS;9IX`hlQX=%[}zwTt?DMZStcEl}Yq^^i$<tXh0D%]#z#8f7zB)salE<P\\>7H\\4[g\\A'N0:,H8\n$9B~[7\r)Y\n6Sg>L3#{\u000b5OP}9tv",
    "+Zd\u000beF\fWX)'1W?K\u000b>q",
    "'qI1m,!oO'21\tL#@\"`\rAg\t4|"
  ]
]

Error (from DynSolType::decode_params): Type check failed for "Solidity pointer (uint32)" with data: 0000000000000000000000000000000000004f89f5d169e8bce841281b02d252

Example 3

Type: (bytes22,(bytes9[],string)[],bytes,uint200,bytes13,address)

Data (as json):

[
  "0x6be25a61350829e7d21728e60c4af483b068f8697cbe",
  [
    [
      [
        "0x3d087033bc44049df8",
        "0x12188bfe07770c84d9"
      ],
      "%r6\"zWyj)XP_B?>._~/J.3!BNCKG^]A,+_X<f@8F\\t?qN\u000bvxl7>nR\u000boq5YNYAEN51kL$cSqP"
    ],
    [
      [
        "0x59df7607fcf965043e",
        "0x99d15c7fa5434787bd"
      ],
      "0g\tEJhQ g&1\fE4|q"
    ]
  ],
  "0xff26530d3839f2c9755525bbd557073e6cbbd29a862f3307cdd5952aa6e318280f45ad3d254747aaafdb51f1a38625477336ecc2771203e6952fe84b474b6ab46a9cbf81fd5f39",
  10252521428253414868099309238095001370685855238315029898719,
  "0x0d751e237aec5d0a891b06633f",
  "0x5824517a27e605ea65cb24c968c75556e7d9a9e2"
]

Error (from DynSolType::decode_params): Buffer overrun while deserializing

@Philogy Philogy added the bug Something isn't working label Jul 2, 2023
@prestwich
Copy link
Member

appreciate it. identified the issue and expect to have a branch this afternoon

@prestwich
Copy link
Member

prestwich commented Jul 2, 2023

issue arises because solidity specification appears to be incorrect wrt encoding of dynamic sequences when compared to real-world implementation

specified behavior (snippet below) is that offset is "relative to the start of enc(X)"

actual behavior in test vectors and other implementations is that the offset is relative to the first word of the body (the second word of the encoding)

Note that in the dynamic case, head(X(i)) is well-defined since the lengths of the head parts only depend on the types and not the values. The value of head(X(i)) is the offset of the beginning of tail(X(i)) relative to the start of enc(X).

...

 T[] where X has k elements (k is assumed to be of type uint256):

enc(X) = enc(k) enc((X[0], ..., X[k-1]))

i.e. it is encoded as if it were a tuple with k elements of the same type (resp. an array of static size k), prefixed with the number of elements.

@Philogy
Copy link
Author

Philogy commented Jul 2, 2023

Not sure I see what you mean? From what I can see other implementations are following the spec?

@prestwich
Copy link
Member

prestwich commented Jul 2, 2023

the spec says the following:

  • for X of type T[] with length k, enc(X) = enc(k) enc((X[0], ..., X[k-1]))
  • for dynamically sized elements in a sequence X, head(X(i)) is the offset of the beginning of tail(X(i)) relative to the start of enc(X).

so based on that we expect the offsets in a dynamically-length sequence to be relative to the beginning of enc(X), which is defined to begin with enc(k)

the actual behavior (as observed in other implementations and in the tests) is that the offsets are relative to the word AFTER enc(k)

@Philogy
Copy link
Author

Philogy commented Jul 2, 2023

I think the use of the = symbol may be misleading in the spec

I interpret the spec such that when it says that objects of type T[] are encoded as enc(X) = enc(k) enc((X[0], ..., X[k-1])) I interpret it as encode the literal k and then initiate a separate new enc context for the tuple (X[0], ..., X[k-1]) therefore the beginning of enc(X) (in the new context) no longer includes enc(k).

I see more evidence for this interpretation by how head(X(i)) is defined: head(X(i)) = enc(len( head(X(1)) ... head(X(k)) tail(X(1)) ... tail(X(i-1)) )), namely it does not include the length of the outer dynamic array in its len(...)

@prestwich
Copy link
Member

prestwich commented Jul 2, 2023

that reading contradicts the following line tho: i.e. it is encoded as if it were a tuple with k elements of the same type (resp. an array of static size k), prefixed with the number of elements., which would read i.e. it is encoded as the 2-tuple of k and a fixed-size sequence containing its elements

not arguing with you, btw. the other implementations have clear behavior even if the spec is unclear.
trying to identify places we could request a specification wording change to make it more clear

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

Successfully merging a pull request may close this issue.

2 participants