readUint16() and readUint64() fail to clean up the returned results.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/UnsafeBytesCalldata.sol#L18-L30

Vulnerability details

Impact

Detailed description of the impact of this finding. readUint16() and readUint64() fail to clean up the returned results.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

readUint16() and readUint64() fail to clean up the returned results, this is because in Assembly, the cleanup is not automatic:

function readUint16(bytes calldata _bytes, uint256 _start) internal pure returns (uint16 result) {
        assembly {
            let offset := sub(_bytes.offset, 30)
            result := calldataload(add(offset, _start))
        }
    }

    function readUint64(bytes calldata _bytes, uint256 _start) internal pure returns (uint64 result) {
        assembly {
            let offset := sub(_bytes.offset, 24)
            result := calldataload(add(offset, _start))
        }
    }

We need to make sure both of them are cleaned by using masking.

Tools Used

VSCode

Recommended Mitigation Steps

We introduce two masks to clean up the results before we return them.

+ uint256 constant MASK16 = 0xffff;
+ uint256 constant MASK64 = 0xffffffffffffffff;

function readUint16(bytes calldata _bytes, uint256 _start) internal pure returns (uint16 result) {
        assembly {
            let offset := sub(_bytes.offset, 30)
            result := calldataload(add(offset, _start))
+            result := and(result, MASK16)           
        }
    }

    function readUint64(bytes calldata _bytes, uint256 _start) internal pure returns (uint64 result) {
        assembly {
            let offset := sub(_bytes.offset, 24)
            result := calldataload(add(offset, _start))
+            result := and(result, MASK64)           
        }
    }

[GalloDaSballo]

Thinking this one is invalid as the offset are 30 => (32 - 30, which are 2 bytes => 2 8 = u16) 24 => (32 - 24, which are 8 bytes => 8 8 = u64)

Flagging in case

[miladpiri]

Masking should be applied by Solidity automatically in the places where the returned values are used.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

@miladpiri flagging this specific test that may help:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.15;

import {Test} from "forge-std/Test.sol";

contract Unmasked {
    event Debug(string name, uint256 value);
    function readUint16(bytes calldata _bytes, uint256 _start) external returns (uint256 result) {
        uint256 offset;

        assembly {
            offset := sub(_bytes.offset, 30)

        }

        emit Debug("offset", offset);

        assembly {
            result := calldataload(add(offset, _start))
        }

    }
}

contract Masked {
    event Debug(string name, uint256 value);

    uint256 constant MASK16 = 0xffff;

    function readUint16(bytes calldata _bytes, uint256 _start) external returns (uint256 result) {
        uint256 offset;

        assembly {
            offset := sub(_bytes.offset, 30)

        }

        emit Debug("offset", offset);

        assembly {
            result := calldataload(add(offset, _start))
            result := and(result, MASK16)
        }

    }
}

contract TestDirty is Test {
    Unmasked u;
    Masked m;

    function setUp() public {
        u = new Unmasked();
        m = new Masked();
    }

    event Debug(string name, uint256 value);

    function testDirty(uint256 value1) public {
        bytes memory toSend1 = abi.encodePacked(value1);

        uint256 v1 = u.readUint16(toSend1, 0);
        uint256 v2 = m.readUint16(toSend1, 0);

        assertEq(v1, v2);
    }
}

Running 1 test for test/testDirty.sol:TestDirty
[FAIL. Reason: Assertion failed. Counterexample: calldata=0xd1558e590000000000000000000000000000000000000000000000000000000000000000, args=[0]] testDirty(uint256) (runs: 0, μ: 0, ~: 0)
Logs:
  Error: a == b not satisfied [uint]
    Expected: 0
      Actual: 2097152

Traces:
  [274645] TestDirty::setUp() 
    ├─ [82529] → new Unmasked@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← 412 bytes of code
    ├─ [83329] → new Masked@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← 416 bytes of code
    └─ ← ()

  [30260] TestDirty::testDirty(0) 
    ├─ [2361] Unmasked::readUint16(0x0000000000000000000000000000000000000000000000000000000000000000, 0) 
    │   ├─ emit Debug(name: offset, value: 70)
    │   └─ ← 2097152
    ├─ [2367] Masked::readUint16(0x0000000000000000000000000000000000000000000000000000000000000000, 0) 
    │   ├─ emit Debug(name: offset, value: 70)
    │   └─ ← 0
    ├─ emit log(: Error: a == b not satisfied [uint])
    ├─ emit log_named_uint(key:   Expected, val: 0)
    ├─ emit log_named_uint(key:     Actual, val: 2097152)
    ├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001) 
    │   └─ ← ()
    └─ ← ()

Test result: FAILED. 0 passed; 1 failed; finished in 14.52ms

Failing tests:
Encountered 1 failing test in test/testDirty.sol:TestDirty
[FAIL. Reason: Assertion failed. Counterexample: calldata=0xd1558e590000000000000000000000000000000000000000000000000000000000000000, args=[0]] testDirty(uint256) (runs: 0, μ: 0, ~: 0)

[GalloDaSballo]

From my experience changing values all return values are non-zero for a random integer, flagging this up to see if this helps

[GalloDaSballo]

Throughout the codebase readUint16 is used to get the length of the dictionary: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L80-L81

            uint256 dictionaryLen = uint256(_rawCompressedData.readUint16(0));

So I believe that getting a 0 value would be incorrect

[chaduke3730]

Maybe add "vm.assume(value1 != 0)"?

[GalloDaSballo]

Maybe add "vm.assume(value1 != 0)"?

Doesn't seem to change much

[GalloDaSballo]

I think this example shows how the mask is already applied by the return value size

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.15;

contract Debug {

    function readUint16(bytes calldata _bytes, uint256 offset, uint256 _start) external view returns (uint16 result) {
        assembly {
            result := calldataload(add(offset, _start))
        }
    }
}

contract Unmasked {
    event DebugBytes(string name, bytes value);
    event Debug(string name, uint256 value);

    function readUint16(bytes calldata _bytes, uint256 _start) external view returns (uint16 result) {
        uint256 offset;
        assembly {
            offset := sub(_bytes.offset, 30)
        }

        assembly {
            result := calldataload(add(offset, _start))
        }
    }
}

contract UnmaskedUnlimited {
    event DebugBytes(string name, bytes value);
    event Debug(string name, uint256 value);

    function readUint16(bytes calldata _bytes, uint256 _start) external view returns (uint256 result) {
        uint256 offset;
        assembly {
            offset := sub(_bytes.offset, 30)
        }

        assembly {
            result := calldataload(add(offset, _start))
        }
    }
}

contract Masked {
    event Debug(string name, uint256 value);

    uint256 constant MASK16 = 0xffff;

    function readUint16(bytes calldata _bytes, uint256 _start) external view returns (uint256 result) {
        uint256 offset;

        assembly {
            offset := sub(_bytes.offset, 30)
            result := calldataload(add(offset, _start))
            result := and(result, MASK16)
        }
    }
}

contract MaskedCapped {
    event Debug(string name, uint256 value);

    uint256 constant MASK16 = 0xffff;

    function readUint16(bytes calldata _bytes, uint256 _start) external view returns (uint16 result) {
        uint256 offset;

        assembly {
            offset := sub(_bytes.offset, 30)
            result := calldataload(add(offset, _start))
            result := and(result, MASK16)
        }
    }
}

Ran in brownie

brownie console

m = Masked.deploy({"from": a[0]})
u = Unmasked.deploy({"from": a[0]})
f = UnmaskedUnlimited.deploy({"from": a[0]})
d = Debug.deploy({"from": a[0]})
c = MaskedCapped.deploy({"from": a[0]})
from brownie.convert import to_bytes

bytes = to_bytes(0xABCDEF12345678, "bytes")

>>> m.readUint16(bytes, 0)

43981
>>> c.readUint16(bytes, 0)
43981
>>> f.readUint16(bytes, 0)

502733
>>> u.readUint16(bytes, 0)

43981

Meaning that the mask is applied by the uint16 return value, meaning the finding is invalid

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid