0x-Settler

Auditing smart contracts that rely heavily on inline assembly presents significant challenges due to the inherently low-level nature of the code and the bypassing of Solidity's built-in safety mechanisms. Such code is optimised for performance and not readability, and introduces the possibility of subtle bugs. The level of confidence which can be achieved in a timeboxed manual review is thus necessarily smaller than that which can be achieved when auditing code written in high level Solidity. Despite these challenges, following extensive analysis and thorough comprehension of the codebase, the auditors conclude that the code maintains exceptionally high quality standards.

When using highly optimized code, one should balance the unavoidable security risk introduced by the code’s complexity (see P1) and the actual gas savings provided by these optimizations. If the savings are substantial then the risk is justified; if, on the other hand, the benefits are marginal or zero, then the risks clearly outweigh the benefits.

Consider, for instance, the heavy use of callvalue(), appearing 88 times in CrossChainReceiverFactory. This is a known micro-optimization technique to employ the literal 0 in a cheaper way, since 0 typically translates to PUSH1 00, costing 3 gas to push 0 into the stack, while callvalue() costs just 2 gas.

However, since Shanghai (that is, for almost 3 years), EVM has a PUSH0 opcode, for pushing the literal 0, which also costs 2 gas, nullifying the use of this technique. So now we have a micro-optimization that has zero benefits post-Shanghai, and marginal benefits for pre-Shanghai chains, and at the same time:

• Makes the code harder to read

• Introduces 88 places in the code that we have to ensure they cannot be reached by any payable execution flow. Any such flow would very likely lead to a serious vulnerability.

For another small example, consider the following code:

function deploy(bytes32 root, bool setOwnerNotCleanup, address initialOwner)
{
    ...
    assembly ("memory-safe") {
        // derive the deployment salt from the owner
        // dedaub: the order matters here!
        mstore(0x14, initialOwner)
        mstore(callvalue(), root)
        let salt := keccak256(callvalue(), 0x34)

This code hashes an address and a bytes32, but it is written in a cryptic way to achieve two micro-optimizations:

First, the code hashes only 52 bytes, instead of 64, avoiding to include the 12 zero bytes of initialOwner. Although keccak256’s cost obviously depends on the input size, both 52 and 64 are rounded to the same word size so there is actually no cost saving at all.

Second, even if we want to hash only 52 bytes, the natural way would be to write the address first, the bytes32 afterwards, and start hashing from byte 12. However the code also wants to micro-optimize the offset, since a 0-offset allows the use of callvalue() (or PUSH0), as discussed above. So it does it in a much more complex way: it writes first the address at offset 20, then the bytes32 at offset 0, hence overwriting the 12 zeros and ending up with the 52 bytes to hash at offset 0.

The result is a complex code for marginal gas benefits, with a subtle downside: the order of the two mstore operations is critical, if at any future update the order changes and root is written first (which is the natural order, write first at offset 0), then initialOwner would overwrite 12 bytes of root with zeros, opening the possibilities of subtle collisions.

Although we do appreciate the engineering effort put into such a highly optimized code, from the point of view of security we recommend simplifying the code as much as possible, keeping only optimizations that truly lead to substantial savings.

One of the main difficulties about building confidence on the security of this protocol is the very general nature of all operations and the lack of very specific execution flows. Calling arbitrary methods on arbitrary contracts is possible, with the selection of the actual calls happening off chain.

To improve this aspect, one could consider designs in which some of the operations are “hardened”, reducing their flexibility and as a consequence also reducing the attack surface, a common security practice. A concrete example is given in L1, discussing a potential permissioned model for deploy, but the general principle could be applied to other operations as well.

The MulticallContext.sol contract has a _msgData(address multicall) function which is supposed to trim the calldata and strip away the sender information when the sender of a call is the multicall contract. However this behaviour seems to have been inverted when the function was updated in the pull request. With the change, the data is stripped when we do not have a multicall, and kept when we have a multicall instead.

function _msgData(address multicall) internal view
returns (bytes calldata r) {
        address sender = super._msgSender();
        r = super._msgData();
        assembly ("memory-safe") {
            r.length :=
                sub(r.length, mul(0x14,
                // Dedaub - lt should be iszero here
                                  lt(0x00, shl(0x60, xor(multicall, sender)))))
        }
    }

CrossChainReceiverFactory::_verifySimpleSignature uses the ecrecover precompile in order to verify a compact ECDSA signature against owner_. The call is performed by highly optimized assembly code:

function _verifySimpleSignature(bytes32 signingHash, bytes calldata rvs, address owner_) private view {
     ...

        mstore(callvalue(), signingHash)
        let vs := calldataload(add(0x20, rvs.offset))
        mstore(0x20, add(0x1b, shr(0xff, vs))) // v
        mstore(0x40, calldataload(rvs.offset)) // r
        mstore(0x60, and(0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff, vs)) // s

        let recovered := mload(staticcall(gas(), 0x01, callvalue(), 0x80, 0x01, 0x20))
        if shl(0x60, xor(owner_, recovered)) {
            mstore(callvalue(), 0x815e1d64) // `InvalidSigner.selector`
            revert(0x1c, 0x04)
        }
        mstore(0x40, ptr)
        mstore(0x60, callvalue())
    }
}

Note that the return value of ecrecover is read at memory offset 0x01. In case of a wrong signature, the precompile succeeds (staticcall returns 1), but no data is returned at all (note that this is the low-level EVM precompile, not the solidity ecrecover). The code, however, does not check returndatasize to find how many bytes are actually returned. Since no data was returned, the check will happen against the previous contents of memory offset 0x01.

Luckily, in the current code this memory offset cannot be fully controlled by the adversary, it will contain the last 31 bytes of signingHash, followed by the first byte of v which is 0x00. So effectively, the semantics of _verifySimpleSignature, instead of:

“reverts unless rvs is a valid signature by owner on signingHash”
becomes:
“reverts unless rvs is a valid signature by owner on signingHash OR the last 19 bytes of signingHash + 0 match owner_”

Although this issue is not easily exploitable, it unnecessarily reduces signature security from 256 to 152bits (and also has the risk that some future changes might make the contents of memory 0x01 easier to control).

For the above reasons, we recommend correcting this behaviour by explicitly checking returndatasize.

CrossChainReceiverFactory::deploy is a permissionless function, allowing anyone to create a proxy contract on behalf of a user. This does not pose a security threat since the owner address is used to compute the proxy address, so anyone can create the proxy but only the owner will have access to the funds.

Nevertheless, it might be advisable to use some permissioned model (eg via signatures) for two reasons:

• First, in order to “harden” the contract, making it more difficult to exploit a discovered vulnerability. Currently, if any vulnerability is found on any method, it is trivial for a malicious user to create a proxy on behalf of any user and exploit the vulnerability. On the other hand, since it is common for proxy contracts to be created and immediately self-destructed, a permissioned deploy could provide some protection even in presence of such vulnerabilities, not allowing the malicious users to exploit non-created proxies.

• A legitimate deploy transaction with setOwnerNotCleanup == false could be caused to fail, if anyone frontruns it (accidentally or maliciously) with a deploy transaction with setOwnerNotCleanup == true, causing the proxy to be created. This is not a critical DoS since the user still has access to the funds; nevertheless, it could create a temporary operational DoS, until some administrator realizes what happened, creating confusion and potentially reducing trust in the protocol.

The function getFromMulticall in CrossChainReceiverFactory.sol can only exit by returning true or reverting. It could therefore be changed into a void function.

The code is compiled with Solidity 0.8.28. Version 0.8.28, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.