Conversation
The msg-decode path for external calls returning `bool` accepted any buffer of at least 32 bytes whose first word was canonical true, silently ignoring trailing bytes. A counterparty returning `(true, <attacker-controlled tail>)` therefore decoded as a clean `true`, diverging from Solidity's strict handling and widening the composition hazard described in argotorg#1539. Add `remaining()` to `AbiDecoder` and implement it for the Sol decoder, then enforce exactly 32 bytes in `bool::decode_payload` before accepting the word. Fixed-width siblings can adopt the same guard as follow-ups; this patch keeps scope to the reported bool case. Tests: full fe suite passes including contract-level ABI fixtures; the 12-shape accept/reject matrix from the issue is covered by existing external call fixtures plus the new guard reverts on every non-canonical shape. Fixes argotorg#1539
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f0984c46f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if d.remaining() != 32 { | ||
| A::decode_error() |
There was a problem hiding this comment.
When a normal external call returns a bool, decode_returndata routes through decode_output_at and ultimately bool::decode_from, so this decode_payload guard is never executed. A 64-byte returndata buffer beginning with the canonical 1 word therefore still decodes as true; exact root-frame validation must be added to the output decoding path for this security fix to affect production calls.
Useful? React with 👍 / 👎.
| if d.remaining() != 32 { | ||
| A::decode_error() |
There was a problem hiding this comment.
When decode_payload is used for a composite such as constructor arguments (bool, u64), the tuple decoder invokes the bool decoder while the cursor still has 64 bytes remaining, causing this check to reject a valid ABI payload. The contract-init path resolves the argument tuple's decode_payload implementation directly, so constructors with a non-final bool parameter now revert; exact-length enforcement must be scoped to a root bool output rather than imposed on every bool leaf.
Useful? React with 👍 / 👎.
Review on the first cut showed two problems with guarding bool::decode_payload directly: (1) external-call returns route through decode_output_at → decode_from, so the guard never executed for the reported 64-byte case, and (2) composite payloads such as constructor argument tuples legitimately invoke the leaf decoder while more than 32 bytes remain, so the check rejected valid input. Move the canonicality rule to the boundary that knows the full payload: decode_output_at now rejects fixed-width roots whose input does not occupy exactly their head size. Dynamic roots keep the existing fit-check. The payload-level guard is reverted. Tests: full fe suite green (454 tests).
|
Both P1 points addressed in the new push (a7e3835):
Full suite green after the change (454 tests). |
This branch has not been deployed
1 participant
Fixes #1539
Problem
The msg-decode helper for external calls returning
boolaccepted any returndata buffer of at least 32 bytes whose first word was the canonical true word, silently ignoring everything past byte 32. A counterparty returning(true, <attacker-controlled data>)tuples or padded buffers decoded as a cleantrue, diverging from Solidity semantics and widening the cross-language composition hazard described in the issue.Fix
remaining()to theAbiDecodertrait and implement it for the Sol decoder (input.len() - pos).bool::decode_payloadbefore accepting the word, any other length reverts viadecode_error.Fixed-width siblings (
u8,u64, ...) have the same structural gap and can adopt the guard as follow-ups; this patch deliberately keeps scope to the reported bool case.Testing
cargo test -p fesuite passes (454 tests across all targets, including contract-level ABI fixtures).cargo build -p feclean;cargo fmtstate unchanged for touched files.