code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

Gas Optimizations #174

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas: Unnecessary ERC20 transfer data length check

The data.length != 0 && data.length >= 32 check has a redundancy. As data.length >= 32 is required and data.length >= 32 implies data.length != 0, one can simplify this expression to data.length >= 32. The YUL version of the code checks "(abi.decode(data, uint256) == 1 && data.length > 31) || data.length == 0" which is correctly optimized.

Gas: Unnecessary conduit runtime code hash computation

The ConduitController computes the _CONDUIT_RUNTIME_CODE_HASH variable by deploying a Conduit contract and reading its .codehash. However, this variable is only used to check if a contract has been deployed by checking conduit.codehash == _CONDUIT_RUNTIME_CODE_HASH. The conduit variable is always a create2 address and by the way create2 addresses work, the check can also be implemented as condit.code.length != 0. (Because the creation code is already part of the create2 address and the creation code always leads to the same runtime code for Conduits.) This saves deploying a Conduit contract in the constructor and the new check should also be more gas-efficient.

Gas: Improve MerkleProof.verify

The MerkleProof.verify function can be improved using the xor trick instead of a switch to order the hashes. Plus some other improvements. Credit

function _verifyProof(
    uint256 leaf,
    uint256 root,
    bytes32[] memory proof
) internal pure {
    bool isValid;

    assembly {
        // Start the hash off as just the starting leaf.
        let computedHash := leaf

        // Get memory start location of the first element in proof array.
        let data := add(proof, OneWord)

        // Iterate over proof elements to compute root hash.
-        for {
-            let end := add(data, mul(mload(proof), OneWord))
-        } lt(data, end) {
-            data := add(data, OneWord)
-        } {
-            // Get the proof element.
-            let loadedData := mload(data)
-
-            // Sort and store proof element and hash.
-            switch gt(computedHash, loadedData)
-            case 0 {
-                mstore(0, computedHash) // Place existing hash first.
-                mstore(0x20, loadedData) // Place new hash next.
-            }
-            default {
-                mstore(0, loadedData) // Place new hash first.
-                mstore(0x20, computedHash) // Place existing hash next.
-            }
-
-            // Derive the updated hash.
-            computedHash := keccak256(0, TwoWords)
-        }
+        for {
+            // Left shift by 5 is equivalent to multiplying by 0x20.
+            let end := add(data, shl(5, mload(proof)))
+        } lt(data, end) {
+            data := add(data, OneWord)
+        } {
+            let loadedData := mload(data)
+            // Slot of `computedHash` in scratch space.
+            // If the condition is true: 0x20, otherwise: 0x00.
+            let scratch := shl(5, gt(computedHash, loadedData))
+
+            // Store elements to hash contiguously in scratch space.
+            // Scratch space is 64 bytes (0x00 - 0x3f) and both elements are 32 bytes.
+            mstore(scratch, computedHash)
+            mstore(xor(scratch, 0x20), loadedData)
+            computedHash := keccak256(0, TwoWords)
+        }

        // Compare the final hash to the supplied root.
        isValid := eq(computedHash, root)
    }

    // Revert if computed hash does not equal supplied root.
    if (!isValid) {
        revert InvalidProof();
    }
}
HardlyDifficult commented 2 years ago

Gas: Unnecessary ERC20 transfer data length check

This impacts the reference implementation which is not a target for optimizations, as the warden notes the optimized Yul version already seems to do the right thing here.

The other 2 recommendations may be worthwhile optimizations to include.