OZ L02; Spearbit 87; Enforce strict abi encoding #346

hensha256 · 2024-09-06T00:18:55Z

No description provided.

Co-authored-by: 0age <[email protected]>

kmbarry1 · 2024-09-06T04:27:22Z

src/libraries/CalldataDecoder.sol

-            let relativeOffset := sub(params.offset, _bytes.offset)
-            // Check that that isn't longer than the bytes themselves, or revert
-            if lt(_bytes.length, add(params.length, relativeOffset)) {
+            for { let offset := 0 } lt(offset, tailOffset) { offset := add(offset, 32) } {


exceedingly minor consistency nit: 32 used here whereas elsewhere the hex 0x20 is used

we love a nit

kmbarry1 · 2024-09-06T04:28:00Z

src/libraries/CalldataDecoder.sol

+                expectedOffset := add(expectedOffset, length)
+            }
+
+            // if the data encoding was invalid, or the provided bytes string isnt as long as the encoding says, revert


isnt --> isn't

kmbarry1 · 2024-09-06T04:31:28Z

src/libraries/CalldataDecoder.sol

+    /// @notice equivalent to SliceOutOfBounds.selector, stored in least-significant bits
+    uint256 constant SLICE_ERROR_SELECTOR = 0x3b99b53d;
+
+    /// @dev equivalent to: abi.decode(params, (bytes, bytes[])) in calldata (requires strict abi encoding)


Might be better to note here that the masking makes it not quite strictly equivalent.

3sGgpQ8H · 2024-09-06T19:04:51Z

src/libraries/CalldataDecoder.sol

+            // Verify actions offset matches strict encoding
+            let invalidData := xor(calldataload(_bytes.offset), 0x40)
+            actions.offset := add(_bytes.offset, 0x60)
+            actions.length := and(calldataload(add(_bytes.offset, 0x40)), OFFSET_OR_LENGTH_MASK)


Here the action.length value is silently trimmed to 32 bytes, which could hide errors in abi encoding. Consider doing instead:

actions.length := calldataload(add(_bytes.offset, 0x40)) invalidData := or(invalidData, shr(32, actions.length))

3sGgpQ8H · 2024-09-06T19:07:58Z

src/libraries/CalldataDecoder.sol

-            let paramsPtr := add(_bytes.offset, calldataload(add(_bytes.offset, 0x20)))
+            // Strict encoding requires that the data begin with:
+            // 0x00: 0x40 (offset to `actions.length`)
+            // 0x20: 0x60 + actions.length (offset to `params.length`)


This comment is inaccurate, as the actions.length value is actually aligned to the word boundary.

3sGgpQ8H · 2024-09-06T19:11:13Z

src/libraries/CalldataDecoder.sol

-            actions.length := calldataload(actionsPtr)
-            params.length := calldataload(paramsPtr)
+            // Round actions length up to be word-aligned, and add 0x60 (for the first 3 words of encoding)
+            let paramsLengthOffset := add(and(add(actions.length, 0x1f), OFFSET_OR_LENGTH_MASK_AND_WORD_ALIGN), 0x60)


Here an intermediate value is silently trimmed to 32 bits, which could hide overflow errors, and could potentially be exploited. Consider doing instead:

let paramsLengthOffset := shl(5, shr(5, add(actions.length, 0x1f))) invalidData := or(invalidData, shr(32, paramsLengthOffset))

3sGgpQ8H · 2024-09-06T19:11:55Z

src/libraries/CalldataDecoder.sol

-            actions.length := calldataload(actionsPtr)
-            params.length := calldataload(paramsPtr)
+            // Round actions length up to be word-aligned, and add 0x60 (for the first 3 words of encoding)
+            let paramsLengthOffset := add(and(add(actions.length, 0x1f), OFFSET_OR_LENGTH_MASK_AND_WORD_ALIGN), 0x60)


As the 0x60 value is added after applying the mask, the result could actually exceed 32 bites.

3sGgpQ8H · 2024-09-06T19:14:02Z

src/libraries/CalldataDecoder.sol

+            // Verify params offset matches strict encoding
+            invalidData := or(invalidData, xor(calldataload(add(_bytes.offset, 0x20)), paramsLengthOffset))
+            let paramsLengthPointer := add(_bytes.offset, paramsLengthOffset)
+            params.length := and(calldataload(paramsLengthPointer), OFFSET_OR_LENGTH_MASK)


Here the params.length value is silently trimmed to 32 bytes, which could hide errors in abi encoding. Consider doing instead:

params.length := calldataload(paramsLengthPointer) invalidData := or(invalidData, shr(32, actions.length))

3sGgpQ8H · 2024-09-06T19:23:25Z

src/libraries/CalldataDecoder.sol

+            // The offset of the `_arg`-th element is `32 * arg`, which stores the offset of the length pointer.
+            // shl(5, x) is equivalent to mul(32, x)
+            let lengthPtr :=
+                add(_bytes.offset, and(calldataload(add(_bytes.offset, shl(5, _arg))), OFFSET_OR_LENGTH_MASK))


Overflow is possible when shifting left and when adding. Consider reverting on overflow.

3sGgpQ8H · 2024-09-06T19:24:49Z

src/libraries/CalldataDecoder.sol

+            // The offset of the `_arg`-th element is `32 * arg`, which stores the offset of the length pointer.
+            // shl(5, x) is equivalent to mul(32, x)
+            let lengthPtr :=
+                add(_bytes.offset, and(calldataload(add(_bytes.offset, shl(5, _arg))), OFFSET_OR_LENGTH_MASK))


Here the lengthPtr value is silently trimmed to 32 bytes, which could hide errors in abi encoding. Consider reverting in case the calculated value exceeds 32 bits.

3sGgpQ8H · 2024-09-06T19:27:32Z

src/libraries/CalldataDecoder.sol

+            let lengthPtr :=
+                add(_bytes.offset, and(calldataload(add(_bytes.offset, shl(5, _arg))), OFFSET_OR_LENGTH_MASK))
+            // the number of bytes in the bytes string
+            length := and(calldataload(lengthPtr), OFFSET_OR_LENGTH_MASK)


Here the length value is silently trimmed to 32 bytes, which could hide errors in abi encoding. Consider reverting in case the length value exceeds 32 bits.

3sGgpQ8H · 2024-09-06T19:27:59Z

src/libraries/CalldataDecoder.sol

    /// @notice Decode the `_arg`-th element in `_bytes` as `bytes`
    /// @param _bytes The input bytes string to extract a bytes string from
    /// @param _arg The index of the argument to extract
    function toBytes(bytes calldata _bytes, uint256 _arg) internal pure returns (bytes calldata res) {
-        (uint256 length, uint256 offset) = toLengthOffset(_bytes, _arg);
+        uint256 length;


This variable is redundant. Just use res.length instead.

3sGgpQ8H · 2024-09-06T19:28:15Z

src/libraries/CalldataDecoder.sol

+            // the number of bytes in the bytes string
+            length := and(calldataload(lengthPtr), OFFSET_OR_LENGTH_MASK)
+            // the offset where the bytes string begins
+            let offset := add(lengthPtr, 0x20)


This variable is redundant. Just use res.offset instead.

Jeiwan · 2024-09-09T08:29:00Z

src/libraries/CalldataDecoder.sol

+            // 0x60: beginning of actions
+
+            // Verify actions offset matches strict encoding
+            let invalidData := xor(calldataload(_bytes.offset), 0x40)


_bytes.offset is always 0, can save some gas here and below. decodeActionsRouterParams is always called on calldata that contains only an encoded ABI.

Jeiwan · 2024-09-09T08:31:21Z

src/libraries/CalldataDecoder.sol

+            // 0x60: beginning of actions
+
+            // Verify actions offset matches strict encoding
+            let invalidData := xor(calldataload(_bytes.offset), 0x40)


Missing test case: failure when the first byte is not 0x40.

Jeiwan · 2024-09-09T08:33:46Z

src/libraries/CalldataDecoder.sol

+            // Round actions length up to be word-aligned, and add 0x60 (for the first 3 words of encoding)
+            let paramsLengthOffset := add(and(add(actions.length, 0x1f), OFFSET_OR_LENGTH_MASK_AND_WORD_ALIGN), 0x60)
+            // Verify params offset matches strict encoding
+            invalidData := or(invalidData, xor(calldataload(add(_bytes.offset, 0x20)), paramsLengthOffset))


A failure here is not tested.

Jeiwan · 2024-09-09T08:35:16Z

src/libraries/CalldataDecoder.sol

+            for { let offset := 0 } lt(offset, tailOffset) { offset := add(offset, 32) } {
+                let itemLengthOffset := calldataload(add(params.offset, offset))
+                // Verify that the offset matches the expected offset from strict encoding
+                invalidData := or(invalidData, xor(itemLengthOffset, expectedOffset))


Not tested when fails.

hensha256 and others added 24 commits August 29, 2024 17:47

improved calldata checks

8f97c52

merge main

31abbbf

remove console

0d5fb80

Merge branch 'main' into better-calldata-checks

ac829a9

remove toLengthOffset function

ca09eff

natspec correction

ae6a374

Merge branch 'main' into better-calldata-checks

2a085f2

merge main

fd3ce50

Merge main

e8cd67f

mask values to be within 32 bits

5eba3fb

verify array elements are encoded in order

a815baf

mask straight after calldata load

c755eed

Merge branch 'main' into better-calldata-checks

a0a4052

merge main

69f556f

masking in toBytes, remove selector constant

2fd239f

fuzz test

25f6434

add testing

ef5b67f

fix for 0 array length

9acd052

merge main

6cffcf2

merge main

c18f1b5

put alt. calldata decoder back (#344)

95cdfdc

Co-authored-by: 0age <[email protected]>

merge main

71709c3

core on main for debug profile

e1501a8

Fix length check

c6397f0

hensha256 changed the base branch from better-calldata-checks to main September 6, 2024 00:19

0age previously approved these changes Sep 6, 2024

View reviewed changes

optimise toBytes

123cef3

hensha256 dismissed 0age’s stale review via 123cef3 September 6, 2024 00:29

linting

5ce4d88

hensha256 mentioned this pull request Sep 6, 2024

OZ L02; Spearbit 87; improved calldata checks #316

Closed

hensha256 changed the title ~~Enforce strict abi encoding~~ OZ L02; Spearbit 87; Enforce strict abi encoding Sep 6, 2024

0age approved these changes Sep 6, 2024

View reviewed changes

hensha256 merged commit 151b282 into main Sep 6, 2024
3 checks passed

hensha256 deleted the 0age-calldata branch September 6, 2024 00:42

kmbarry1 reviewed Sep 6, 2024

View reviewed changes

3sGgpQ8H reviewed Sep 6, 2024

View reviewed changes

Jeiwan reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OZ L02; Spearbit 87; Enforce strict abi encoding #346

OZ L02; Spearbit 87; Enforce strict abi encoding #346

hensha256 commented Sep 6, 2024

kmbarry1 Sep 6, 2024

hensha256 Sep 6, 2024

kmbarry1 Sep 6, 2024

kmbarry1 Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

3sGgpQ8H Sep 6, 2024

Jeiwan Sep 9, 2024

Jeiwan Sep 9, 2024

Jeiwan Sep 9, 2024

Jeiwan Sep 9, 2024

OZ L02; Spearbit 87; Enforce strict abi encoding #346

OZ L02; Spearbit 87; Enforce strict abi encoding #346

Conversation

hensha256 commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment