Clones Data Copying is incorrectly offset by 2 if using `swont@main` instead of `swont@master`

Low/Info issue submitted by GalloDaSballo

Clones Data Copying when using swont main will incorrectly offset by 2 making all data wrong

This is a courtesy report to also prevent this from being sent as valid in the contest

I was able to find this by cloning the repo and using unpinned dependencies

But if we use pinned dependencies then there is no risk

NOTE: This finding applies only when updating dependencies

The default branch for swont is main: https://github.com/Saw-mon-and-Natalie/clones-with-immutable-args/blob/main/src/Clone.sol

OP is using a fork from master: https://github.com/Saw-mon-and-Natalie/clones-with-immutable-args/blob/master/src/Clone.sol

Switching these versions changes some of the logic and will cause the following issue:

Please see the original code, which was never edited over 2 years https://github.com/wighawag/clones-with-immutable-args/blame/master/src/Clone.sol#L85

The edited code, was edited to use hex, the value is correct 240, however, the add was removed

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/libraries/Clone.sol#L90-L96


    /// @return offset The offset of the packed immutable args in calldata
    function _getImmutableArgsOffset() internal pure returns (uint256 offset) {
        assembly {
            offset := sub(calldatasize(), shr(0xf0, calldataload(sub(calldatasize(), 2))))
        }
    }

You can see all shifted values via the following POC

POC Output

    ├─ [421] GuttedDisputeGameFactory::getParentHash()
    │   └─ ← [Return] 0x378a557082e696954a41106e3f108897c39596d752e8d21e86dae0f905024ba3
    ├─ emit DebugBytes32(: "l1HeadOriginal", : 0x378a557082e696954a41106e3f108897c39596d752e8d21e86dae0f905024ba3)
    ├─ emit DebugBytes32(: "l1HeadDecoded", : 0x557082e696954a41106e3f108897c39596d752e8d21e86dae0f905024ba30000)
    ├─ emit DebugBytes32(: "rootOriginal", : 0x000000000000000000000000000000000000000000000000000000000000007b)
    ├─ emit DebugBytes32(: "rootDecoded", : 0x00000000000000000000000000000000000000000000000000000000007b378a)
    └─ ← [Stop]

POC Code

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

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

import { ClonesWithImmutableArgs } from "@cwia/ClonesWithImmutableArgs.sol";
import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import { ISemver } from "src/universal/ISemver.sol";

import { IDisputeGame } from "src/dispute/interfaces/IDisputeGame.sol";
import { IDisputeGameFactory } from "src/dispute/interfaces/IDisputeGameFactory.sol";

import { LibGameId } from "src/dispute/lib/LibGameId.sol";

import { Clone } from "src/libraries/Clone.sol";

import "src/libraries/DisputeTypes.sol";
import "src/libraries/DisputeErrors.sol";

contract SimpleClone is Clone {

  function initialize() public payable virtual {
        assembly { /// @audit Seems impossible due to abi encoding, cause bytes would be [LENGTH, DATA] so it will always be at least 64 bytes
        if gt(calldatasize(), 0x66) {
            // Store the selector for `ExtraDataTooLong()` & revert
            mstore(0x00, 0xc407e025)
            revert(0x1C, 0x04)
        }
    }
  }
    function rootClaim() public pure returns (Claim rootClaim_) {
        rootClaim_ = Claim.wrap(_getArgFixedBytes(0x00));
    }

    function l1Head() public pure returns (Hash l1Head_) {
        l1Head_ = Hash.wrap(_getArgFixedBytes(0x20));
    }

    function extraData() public pure returns (bytes memory extraData_) {
        // The extra data starts at the second word within the cwia calldata and
        // is 32 bytes long.
        extraData_ = _getArgDynBytes(0x40, 0x20);
    }
}

contract GuttedDisputeGameFactory {
  using ClonesWithImmutableArgs for address;

  address impl;

  constructor(address _impl) {
    impl = _impl;
  }

  function getParentHash() public returns (bytes32) {
     bytes32 parentHash = keccak256(abi.encode("123123")); /// @audit NOTE: We put a random value

     return parentHash;
  }

  function createSimplified(
        GameType _gameType, /// @audit Effectively just U256 I think TODO
        Claim _rootClaim,
        bytes calldata _extraData
    )
        external
        payable
        returns (IDisputeGame proxy_)
    {
        // Get the hash of the parent block.
        // bytes32 parentHash = blockhash(block.number - 1); /// @audit Reorg? 
        bytes32 parentHash = getParentHash();

        // Clone the implementation contract and initialize it with the given parameters. /// @audit Can rootClaim tell it that it is longer than what it is?
        proxy_ = IDisputeGame(address(impl).clone(abi.encodePacked(_rootClaim, parentHash, _extraData))); /// @audit why packed like that?
        proxy_.initialize{ value: msg.value }();
    }
}

contract FactoryTests is Test {

  event DebugBytes32(string, bytes32);
  event DebugBytes(string, bytes);
  event DebugVal(string, uint256);
  function testEncodeDecodeFromCloneIsBroken() public {
    SimpleClone impl = new SimpleClone();
    GuttedDisputeGameFactory factory = new GuttedDisputeGameFactory(address(impl));

    // NOTE: Will fail for encode(string), is fine on encode on simple type
    bytes memory encoded = abi.encode(uint256(12));
    emit DebugBytes("encoded", encoded);

    bytes32 rootOriginal = bytes32(uint256(123));

    address clone = address(factory.createSimplified(GameType.wrap(1), Claim.wrap(rootOriginal), encoded));

    bytes memory decoded = SimpleClone(clone).extraData();
    emit DebugBytes("decoded", decoded);

    uint256 original = abi.decode(decoded, (uint256));
    emit DebugVal("original", original);

    bytes32 l1HeadDecoded = Hash.unwrap(SimpleClone(clone).l1Head());
    bytes32 rootDecoded = Claim.unwrap(SimpleClone(clone).rootClaim());
    emit DebugBytes32("l1HeadOriginal", factory.getParentHash());
    emit DebugBytes32("l1HeadDecoded", l1HeadDecoded);
    emit DebugBytes32("rootOriginal", rootOriginal);
    emit DebugBytes32("rootDecoded", rootDecoded);
  }

}

Mitigation

Changing the line back to the original will fix this bug

    function _getImmutableArgsOffset() internal pure returns (uint256 offset) {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            offset := sub(
                calldatasize(),
                add(shr(240, calldataload(sub(calldatasize(), 2))), 2)
            )
        }
    }

sherlock-audit / 2024-02-optimism-2024-judging