feat(ctb): go-fuzz package

.changeset/spicy-buckets-beam.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-bedrock': minor
+---
+
+Refactors the MerkleTrie get function to throw explicitly instead of returning an existence boolean

.changeset/tall-pets-punch.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Adds the `go-fuzz` package and a fuzz mode for Bedrock's `MerkleTrie.sol`

go.work

@@ -17,6 +17,7 @@ use (
 	./op-proposer
 	./op-service
 	./proxyd
+	./packages/contracts-bedrock/go-fuzz
 )
 
 replace github.com/ethereum/go-ethereum v1.10.26 => github.com/ethereum-optimism/op-geth v0.0.0-20221104231810-30db39cae2be

packages/contracts-bedrock/.gas-snapshot

@@ -133,6 +133,27 @@ LegacyERC20ETH_Test:test_mint() (gas: 10649)
 LegacyERC20ETH_Test:test_transfer() (gas: 10711)
 LegacyERC20ETH_Test:test_transferFrom() (gas: 12887)
 LegacyMessagePasser_Test:test_LegacyMessagePasser_passMessageToL1_Succeeds() (gas: 34519)
+MerkleTrie_Test:test_get_reverts_corruptedProof() (gas: 5686)
+MerkleTrie_Test:test_get_reverts_extraProofElements() (gas: 60560)
+MerkleTrie_Test:test_get_reverts_invalidDataRemainder() (gas: 35827)
+MerkleTrie_Test:test_get_reverts_invalidInternalNodeHash() (gas: 50763)
+MerkleTrie_Test:test_get_reverts_nonexistentKey1() (gas: 59624)
+MerkleTrie_Test:test_get_reverts_nonexistentKey2() (gas: 23359)
+MerkleTrie_Test:test_get_reverts_smallerPathThanKey1() (gas: 53476)
+MerkleTrie_Test:test_get_reverts_smallerPathThanKey2() (gas: 54913)
+MerkleTrie_Test:test_get_reverts_wrongKeyProof() (gas: 53844)
+MerkleTrie_Test:test_get_reverts_zeroBranchValueLength() (gas: 43222)
+MerkleTrie_Test:test_get_reverts_zeroLengthKey() (gas: 15886)
+MerkleTrie_Test:test_get_validProofSucceeds1() (gas: 61619)
+MerkleTrie_Test:test_get_validProofSucceeds10() (gas: 50546)
+MerkleTrie_Test:test_get_validProofSucceeds2() (gas: 71531)
+MerkleTrie_Test:test_get_validProofSucceeds3() (gas: 32779)
+MerkleTrie_Test:test_get_validProofSucceeds4() (gas: 23553)
+MerkleTrie_Test:test_get_validProofSucceeds5() (gas: 84215)
+MerkleTrie_Test:test_get_validProofSucceeds6() (gas: 72991)
+MerkleTrie_Test:test_get_validProofSucceeds7() (gas: 79670)
+MerkleTrie_Test:test_get_validProofSucceeds8() (gas: 50546)
+MerkleTrie_Test:test_get_validProofSucceeds9() (gas: 50501)
 OptimismMintableERC20_Test:test_bridge() (gas: 7599)
 OptimismMintableERC20_Test:test_burn() (gas: 51031)
 OptimismMintableERC20_Test:test_burnRevertsFromNotBridge() (gas: 11164)
@@ -151,27 +172,27 @@ OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10
 OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15789)
 OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_success() (gas: 16033)
 OptimismPortalUpgradeable_Test:test_upgradeToAndCall_upgrading_success() (gas: 180435)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 199706)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 202003)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39656)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 197092)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 197814)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 177851)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 236156)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 237807)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 229466)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 332126)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193794)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83498)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50732)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 136758)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() (gas: 277056)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 189117)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 179324)
-OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17276)
-OptimismPortal_Test:test_OptimismPortalReceiveEth_success() (gas: 127480)
-OptimismPortal_Test:test_depositTransaction_NoValueContract_success() (gas: 76681)
-OptimismPortal_Test:test_depositTransaction_NoValueEOA_success() (gas: 77153)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 198517)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 200739)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39634)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 196193)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 194402)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 174437)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 234659)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 238957)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 230626)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 333263)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 194807)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 85539)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidWithdrawalProof_reverts() (gas: 137224)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50754)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() (gas: 277238)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 190171)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 180360)
+OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17298)
+OptimismPortal_Test:test_OptimismPortalReceiveEth_success() (gas: 127483)
+OptimismPortal_Test:test_depositTransaction_NoValueContract_success() (gas: 76706)
+OptimismPortal_Test:test_depositTransaction_NoValueEOA_success() (gas: 76984)
 OptimismPortal_Test:test_depositTransaction_contractCreation_reverts() (gas: 14245)
 OptimismPortal_Test:test_depositTransaction_createWithZeroValueForContract_success() (gas: 76707)
 OptimismPortal_Test:test_depositTransaction_createWithZeroValueForEOA_success() (gas: 77029)

packages/contracts-bedrock/.gitignore

@@ -10,3 +10,4 @@ src/contract-artifacts.ts
 tmp-artifacts
 deployments/mainnet-forked
 deploy-config/mainnet-forked.json
+go-fuzz/fuzz

packages/contracts-bedrock/contracts/libraries/trie/MerkleTrie.sol

@@ -54,11 +54,6 @@ library MerkleTrie {
      */
     uint8 internal constant PREFIX_LEAF_ODD = 3;
 
-    /**
-     * @notice RLP representation of `NULL`.
-     */
-    bytes internal constant RLP_NULL = hex"80";
-
     /**
      * @notice Verifies a proof that a given key/value pair is present in the trie.
      *
@@ -78,8 +73,7 @@ library MerkleTrie {
         bytes[] memory _proof,
         bytes32 _root
     ) internal pure returns (bool) {
-        (bool exists, bytes memory value) = get(_key, _proof, _root);
-        return (exists && Bytes.equal(_value, value));
+        return Bytes.equal(_value, get(_key, _proof, _root));
     }
 
     /**
@@ -89,71 +83,27 @@ library MerkleTrie {
      * @param _proof Merkle trie inclusion proof for the key.
      * @param _root  Known root of the Merkle trie.
      *
-     * @return Whether or not the key exists.
      * @return Value of the key if it exists.
      */
     function get(
         bytes memory _key,
         bytes[] memory _proof,
         bytes32 _root
-    ) internal pure returns (bool, bytes memory) {
+    ) internal pure returns (bytes memory) {
         TrieNode[] memory proof = _parseProof(_proof);
-        (uint256 pathLength, bytes memory keyRemainder, bool isFinalNode) = _walkNodePath(
-            proof,
-            _key,
-            _root
-        );
-
-        bool noRemainder = keyRemainder.length == 0;
-
-        require(noRemainder || isFinalNode, "MerkleTrie: provided proof is invalid");
-
-        bytes memory value = noRemainder ? _getNodeValue(proof[pathLength - 1]) : bytes("");
-
-        return (value.length > 0, value);
-    }
-
-    /**
-     * @notice Walks through a proof using a provided key.
-     *
-     * @param _proof Inclusion proof to walk through.
-     * @param _key   Key to use for the walk.
-     * @param _root  Known root of the trie.
-     *
-     * @return Length of the final path
-     * @return Portion of the key remaining after the walk.
-     * @return Whether or not we've hit a dead end.
-     */
-    // solhint-disable-next-line code-complexity
-    function _walkNodePath(
-        TrieNode[] memory _proof,
-        bytes memory _key,
-        bytes32 _root
-    )
-        private
-        pure
-        returns (
-            uint256,
-            bytes memory,
-            bool
-        )
-    {
-        uint256 pathLength = 0;
         bytes memory key = Bytes.toNibbles(_key);
-
         bytes memory currentNodeID = abi.encodePacked(_root);
         uint256 currentKeyIndex = 0;
-        uint256 currentKeyIncrement = 0;
-        TrieNode memory currentNode;
 
         // Proof is top-down, so we start at the first element (root).
-        for (uint256 i = 0; i < _proof.length; i++) {
-            currentNode = _proof[i];
-            currentKeyIndex += currentKeyIncrement;
+        for (uint256 i = 0; i < proof.length; i++) {
+            TrieNode memory currentNode = proof[i];
 
-            // Keep track of the proof elements we actually need.
-            // It's expensive to resize arrays, so this simply reduces gas costs.
-            pathLength += 1;
+            // Key index should never exceed total key length or we'll be out of bounds.
+            require(
+                currentKeyIndex <= key.length,
+                "MerkleTrie: key index exceeds total key length"
+            );
 
             if (currentKeyIndex == 0) {
                 // First proof element is always the root node.
@@ -177,17 +127,31 @@ library MerkleTrie {
 
             if (currentNode.decoded.length == BRANCH_NODE_LENGTH) {
                 if (currentKeyIndex == key.length) {
-                    // We've hit the end of the key
-                    // meaning the value should be within this branch node.
-                    break;
+                    // Value is the last element of the decoded list (for branch nodes). There's
+                    // some ambiguity in the Merkle trie specification because bytes(0) is a
+                    // valid value to place into the trie, but for branch nodes bytes(0) can exist
+                    // even when the value wasn't explicitly placed there. Geth treats a value of
+                    // bytes(0) as "key does not exist" and so we do the same.
+                    bytes memory value = RLPReader.readBytes(currentNode.decoded[TREE_RADIX]);
+                    require(
+                        value.length > 0,
+                        "MerkleTrie: value length must be greater than zero (branch)"
+                    );
+
+                    // Extra proof elements are not allowed.
+                    require(
+                        i == proof.length - 1,
+                        "MerkleTrie: value node must be last node in proof (branch)"
+                    );
+
+                    return value;
                 } else {
                     // We're not at the end of the key yet.
                     // Figure out what the next node ID should be and continue.
                     uint8 branchKey = uint8(key[currentKeyIndex]);
                     RLPReader.RLPItem memory nextNode = currentNode.decoded[branchKey];
                     currentNodeID = _getNodeID(nextNode);
-                    currentKeyIncrement = 1;
-                    continue;
+                    currentKeyIndex += 1;
                 }
             } else if (currentNode.decoded.length == LEAF_OR_EXTENSION_NODE_LENGTH) {
                 bytes memory path = _getNodePath(currentNode);
@@ -197,38 +161,49 @@ library MerkleTrie {
                 bytes memory keyRemainder = Bytes.slice(key, currentKeyIndex);
                 uint256 sharedNibbleLength = _getSharedNibbleLength(pathRemainder, keyRemainder);
 
+                // Whether this is a leaf node or an extension node, the path remainder MUST be a
+                // prefix of the key remainder (or be equal to the key remainder) or the proof is
+                // considered invalid.
                 require(
-                    keyRemainder.length >= pathRemainder.length,
-                    "MerkleTrie: invalid key length for leaf or extension node"
+                    pathRemainder.length == sharedNibbleLength,
+                    "MerkleTrie: path remainder must share all nibbles with key"
                 );
 
                 if (prefix == PREFIX_LEAF_EVEN || prefix == PREFIX_LEAF_ODD) {
-                    if (
-                        pathRemainder.length == sharedNibbleLength &&
-                        keyRemainder.length == sharedNibbleLength
-                    ) {
-                        // The key within this leaf matches our key exactly.
-                        // Increment the key index to reflect that we have no remainder.
-                        currentKeyIndex += sharedNibbleLength;
-                    }
+                    // Prefix of 2 or 3 means this is a leaf node. For the leaf node to be valid,
+                    // the key remainder must be exactly equal to the path remainder. We already
+                    // did the necessary byte comparison, so it's more efficient here to check that
+                    // the key remainder length equals the shared nibble length, which implies
+                    // equality with the path remainder (since we already did the same check with
+                    // the path remainder and the shared nibble length).
+                    require(
+                        keyRemainder.length == sharedNibbleLength,
+                        "MerkleTrie: key remainder must be identical to path remainder"
+                    );
+
+                    // Our Merkle Trie is designed specifically for the purposes of the Ethereum
+                    // state trie. Empty values are not allowed in the state trie, so we can safely
+                    // say that if the value is empty, the key should not exist and the proof is
+                    // invalid.
+                    bytes memory value = RLPReader.readBytes(currentNode.decoded[1]);
+                    require(
+                        value.length > 0,
+                        "MerkleTrie: value length must be greater than zero (leaf)"
+                    );
 
-                    // We've hit a leaf node, so our next node should be NULL.
-                    currentNodeID = RLP_NULL;
-                    break;
+                    // Extra proof elements are not allowed.
+                    require(
+                        i == proof.length - 1,
+                        "MerkleTrie: value node must be last node in proof (leaf)"
+                    );
+
+                    return value;
                 } else if (prefix == PREFIX_EXTENSION_EVEN || prefix == PREFIX_EXTENSION_ODD) {
-                    if (sharedNibbleLength != pathRemainder.length) {
-                        // Our extension node is not identical to the remainder.
-                        // We've hit the end of this path
-                        // updates will need to modify this extension.
-                        currentNodeID = RLP_NULL;
-                        break;
-                    } else {
-                        // Our extension shares some nibbles.
-                        // Carry on to the next node.
-                        currentNodeID = _getNodeID(currentNode.decoded[1]);
-                        currentKeyIncrement = sharedNibbleLength;
-                        continue;
-                    }
+                    // Prefix of 0 or 1 means this is an extension node. We move onto the next node
+                    // in the proof and increment the key index by the length of the path remainder
+                    // which is equal to the shared nibble length.
+                    currentNodeID = _getNodeID(currentNode.decoded[1]);
+                    currentKeyIndex += sharedNibbleLength;
                 } else {
                     revert("MerkleTrie: received a node with an unknown prefix");
                 }
@@ -237,11 +212,7 @@ library MerkleTrie {
             }
         }
 
-        return (
-            pathLength,
-            Bytes.slice(key, currentKeyIndex),
-            Bytes.equal(currentNodeID, RLP_NULL)
-        );
+        revert("MerkleTrie: ran out of proof elements");
     }
 
     /**
@@ -287,17 +258,6 @@ library MerkleTrie {
         return Bytes.toNibbles(RLPReader.readBytes(_node.decoded[0]));
     }
 
-    /**
-     * @notice Gets the value for a node.
-     *
-     * @param _node Node to get a value for.
-     *
-     * @return Node value, as hex bytes.
-     */
-    function _getNodeValue(TrieNode memory _node) private pure returns (bytes memory) {
-        return RLPReader.readBytes(_node.decoded[_node.decoded.length - 1]);
-    }
-
     /**
      * @notice Utility; determines the number of nibbles shared between two nibble arrays.
      *

packages/contracts-bedrock/contracts/libraries/trie/SecureMerkleTrie.sol

@@ -40,14 +40,13 @@ library SecureMerkleTrie {
      * @param _proof Merkle trie inclusion proof for the key.
      * @param _root  Known root of the Merkle trie.
      *
-     * @return Whether or not the key exists.
      * @return Value of the key if it exists.
      */
     function get(
         bytes memory _key,
         bytes[] memory _proof,
         bytes32 _root
-    ) internal pure returns (bool, bytes memory) {
+    ) internal pure returns (bytes memory) {
         bytes memory key = _getSecureKey(_key);
         return MerkleTrie.get(key, _proof, _root);
     }