Clarified usage of CREATE

CREATE was originally intended for gas savings. While one sketch did move to
CREATE2, the security concerns around address collisions (requiring all init
codes not be malleable to achieve security) continue to justify this.

To resolve the gas estimation concerns raised in the prior commit, the
createAddress function has been made constant-gas.

processor/ethereum/router/build.rs

@@ -32,6 +32,17 @@ fn main() {
     &artifacts_path,
   )
   .unwrap();
+  // These are detected multiple times and distinguished, hence their renaming to canonical forms
+  fs::rename(
+    artifacts_path.clone() + "/Router_sol_Router.bin",
+    artifacts_path.clone() + "/Router.bin",
+  )
+  .unwrap();
+  fs::rename(
+    artifacts_path.clone() + "/Router_sol_Router.bin-runtime",
+    artifacts_path.clone() + "/Router.bin-runtime",
+  )
+  .unwrap();
 
   // This cannot be handled with the sol! macro. The Router requires an import
   // https://github.com/alloy-rs/core/issues/602
@@ -44,11 +55,16 @@ fn main() {
     &(artifacts_path.clone() + "/router.rs"),
   );
 
+  let test_artifacts_path = artifacts_path + "/tests";
+  if !fs::exists(&test_artifacts_path).unwrap() {
+    fs::create_dir(&test_artifacts_path).unwrap();
+  }
+
   // Build the test contracts
   build_solidity_contracts::build(
     &["../../../networks/ethereum/schnorr/contracts", "../erc20/contracts", "contracts"],
     "contracts/tests",
-    &(artifacts_path + "/tests"),
+    &test_artifacts_path,
   )
   .unwrap();
 }

processor/ethereum/router/contracts/Router.sol

@@ -35,6 +35,17 @@ contract Router is IRouterWithoutCollisions {
   /// @dev The address in transient storage used for the reentrancy guard
   bytes32 constant REENTRANCY_GUARD_SLOT = bytes32(uint256(keccak256("ReentrancyGuard Router")) - 1);
 
+  /**
+   * @dev The next nonce used to determine the address of contracts deployed with CREATE. This is
+   *  used to predict the addresses of deployed contracts ahead of time.
+   */
+  /*
+    We don't expose a getter for this as it shouldn't be expected to have any specific value at a
+    given moment in time. If someone wants to know the address of their deployed contract, they can
+    have it emit an event and verify the emitting contract is the expected one.
+  */
+  uint256 private _smartContractNonce;
+
   /**
    * @dev The nonce to verify the next signature with, incremented upon an action to prevent
    *  replays/out-of-order execution
@@ -53,17 +64,6 @@ contract Router is IRouterWithoutCollisions {
    */
   bytes32 private _seraiKey;
 
-  /**
-   * @dev The next nonce used to determine the address of contracts deployed with CREATE. This is
-   *   used to predict the addresses of deployed contracts ahead of time.
-   */
-  /*
-    We don't expose a getter for this as it shouldn't be expected to have any specific value at a
-    given moment in time. If someone wants to know the address of their deployed contract, they can
-    have it emit an event and verify the emitting contract is the expected one.
-  */
-  uint64 private _smartContractNonce;
-
   /// @dev The address escaped to
   address private _escapedTo;
 
@@ -122,8 +122,7 @@ contract Router is IRouterWithoutCollisions {
   }
 
   /**
-   * @dev
-   *   Verify a signature of the calldata, placed immediately after the function selector. The
+   * @dev Verify a signature of the calldata, placed immediately after the function selector. The
    *  calldata should be signed with the nonce taking the place of the signature's commitment to
    *  its nonce, and the signature solution zeroed.
    */
@@ -409,45 +408,57 @@ contract Router is IRouterWithoutCollisions {
 
   /// @notice Calculate the next address which will be deployed to by CREATE
   /**
-   * @dev This manually implements the RLP encoding to save gas over the usage of CREATE2. While the
-   *   the keccak256 call itself is surprisingly cheap, the memory cost (quadratic and already
-   *   detrimental to other `OutInstruction`s within the same batch) is sufficiently concerning to
-   *   justify this.
+   * @dev While CREATE2 is preferable inside smart contracts, CREATE2 is fundamentally vulnerable to
+   *  collisions. Our usage of CREATE forces an incremental nonce infeasible to brute force. While
+   *  addresses are still variable to the Router address, the Router address itself is the product
+   *  of an incremental nonce (the Deployer's). The Deployer's address is constant (generated via
+   *  NUMS methods), finally ensuring the security of this.
+   *
+   *  This is written to be constant-gas, allowing state-independent gas prediction.
+   *
+   *  This has undefined behavior when `nonce` is zero (EIP-161 makes this irrelevant).
    */
   function createAddress(uint256 nonce) internal view returns (address) {
     unchecked {
-      /*
-        The hashed RLP-encoding is:
-          - Header (1 byte)
-          - Address header (1 bytes)
-          - Address (20 bytes)
-          - Nonce (1 ..= 9 bytes)
-        Since the maximum length is less than 32 bytes, we calculate this on the stack.
-      */
-      // Shift the address from bytes 12 .. 32 to 2 .. 22
-      uint256 rlpEncoding = uint256(uint160(address(this))) << 80;
-      uint256 rlpEncodingLen;
-      if (nonce <= 0x7f) {
-        // 22 + 1
-        rlpEncodingLen = 23;
-        // Shift from byte 31 to byte 22
-        rlpEncoding |= (nonce << 72);
-      } else {
-        uint256 bitsNeeded = 8;
-        while (nonce >= (1 << bitsNeeded)) {
-          bitsNeeded += 8;
+      // The amount of bytes needed to represent the nonce
+      uint256 bitsNeeded = 0;
+      for (uint256 bits = 0; bits <= 64; bits += 8) {
+        bool valueFits = nonce < (uint256(1) << bits);
+        bool notPriorSet = bitsNeeded == 0;
+        // If the value fits, and the bits weren't prior set, we should set the bits now
+        uint256 shouldSet;
+        // slither-disable-next-line assembly
+        assembly {
+          shouldSet := and(valueFits, notPriorSet)
+        }
+        // Carry the existing bitsNeeded value, set bits if should set
+        bitsNeeded = bitsNeeded + (shouldSet * bits);
       }
       uint256 bytesNeeded = bitsNeeded / 8;
-        // 22 + 1 + the amount of bytes needed
-        rlpEncodingLen = 23 + bytesNeeded;
-        // Shift from byte 31 to byte 22
-        rlpEncoding |= (0x80 + bytesNeeded) << 72;
-        // Shift past the unnecessary bytes
-        rlpEncoding |= nonce << (72 - bitsNeeded);
+
+      // if the nonce is an RLP string or not
+      bool nonceIsNotStringBool = nonce <= 0x7f;
+      uint256 nonceIsNotString;
+      // slither-disable-next-line assembly
+      assembly {
+        nonceIsNotString := nonceIsNotStringBool
       }
-      rlpEncoding |= ADDRESS_HEADER;
+      uint256 nonceIsString = nonceIsNotString ^ 1;
+
+      // Define the RLP length
+      uint256 rlpEncodingLen = 23 + (nonceIsString * bytesNeeded);
+
+      uint256 rlpEncoding =
       // The header, which does not include itself in its length, shifted into the first byte
-      rlpEncoding |= (0xc0 + (rlpEncodingLen - 1)) << 248;
+      ((0xc0 + (rlpEncodingLen - 1)) << 248)
+      // The address header, which is constant
+      | ADDRESS_HEADER
+      // Shift the address from bytes 12 .. 32 to 2 .. 22
+      | (uint256(uint160(address(this))) << 80)
+      // Shift the nonce (one byte) or the nonce's header from byte 31 to byte 22
+      | (((nonceIsNotString * nonce) + (nonceIsString * (0x80 + bytesNeeded))) << 72)
+      // Shift past the unnecessary bytes
+      | (nonce * nonceIsString) << (72 - bitsNeeded);
 
       // Store this to the scratch space
       bytes memory rlp;
@@ -495,6 +506,8 @@ contract Router is IRouterWithoutCollisions {
    *  of CEI with `verifySignature` prevents replays, re-entrancy would allow out-of-order
    *  completion for the execution of batches (despite their in-order start of execution) which
    *  isn't a headache worth dealing with.
+   *
+   * Re-entrancy is also explicitly required due to how `_smartContractNonce` is handled.
    */
   // @param signature The signature by the current key for Serai's Ethereum validators
   // @param coin The coin all of these `OutInstruction`s are for
@@ -536,7 +549,7 @@ contract Router is IRouterWithoutCollisions {
             call the contract again.
 
             We use CREATE, not CREATE2, despite the difficulty in calculating the address
-            in-contract, for cost-savings reasons explained within `createAddress`'s documentation.
+            in-contract, for reasons explained within `createAddress`'s documentation.
           */
           address nextAddress = createAddress(_smartContractNonce);
           success = erc20TransferOut(nextAddress, coin, outs[i].amount);

processor/ethereum/router/contracts/tests/CreateAddress.sol

@@ -5,7 +5,7 @@ import "Router.sol";
 
 // Wrap the Router with a contract which exposes the address
 contract CreateAddress is Router {
-  constructor() Router(bytes32(uint256(1))) {}
+  constructor() Router(bytes32(uint256(1))) { }
 
   function createAddressForSelf(uint256 nonce) external returns (address) {
     return Router.createAddress(nonce);

processor/ethereum/router/src/gas.rs

@@ -23,8 +23,9 @@ const CHAIN_ID: U256 = U256::from_be_slice(&[1]);
 pub(crate) type GasEstimator = Evm<'static, (), InMemoryDB>;
 
 impl Router {
-  const NONCE_STORAGE_SLOT: U256 = U256::from_be_slice(&[0]);
-  const SERAI_KEY_STORAGE_SLOT: U256 = U256::from_be_slice(&[2]);
+  const SMART_CONTRACT_NONCE_STORAGE_SLOT: U256 = U256::from_be_slice(&[0]);
+  const NONCE_STORAGE_SLOT: U256 = U256::from_be_slice(&[1]);
+  const SERAI_KEY_STORAGE_SLOT: U256 = U256::from_be_slice(&[3]);
 
   // Gas allocated for ERC20 calls
   #[cfg(test)]
@@ -46,11 +47,11 @@ impl Router {
     the correct set of prices for the network they're operating on.
   */
   /// The gas used by `confirmSeraiKey`.
-  pub const CONFIRM_NEXT_SERAI_KEY_GAS: u64 = 57_764;
+  pub const CONFIRM_NEXT_SERAI_KEY_GAS: u64 = 57_736;
   /// The gas used by `updateSeraiKey`.
-  pub const UPDATE_SERAI_KEY_GAS: u64 = 60_073;
+  pub const UPDATE_SERAI_KEY_GAS: u64 = 60_045;
   /// The gas used by `escapeHatch`.
-  pub const ESCAPE_HATCH_GAS: u64 = 44_037;
+  pub const ESCAPE_HATCH_GAS: u64 = 61_094;
 
   /// The key to use when performing gas estimations.
   ///
@@ -89,6 +90,15 @@ impl Router {
         },
       );
 
+      // Insert the value for _smartContractNonce set in the constructor
+      // All operations w.r.t. execute in constant-time, making the actual value irrelevant
+      db.insert_account_storage(
+        self.address,
+        Self::SMART_CONTRACT_NONCE_STORAGE_SLOT,
+        U256::from(1),
+      )
+      .unwrap();
+
       // Insert a non-zero nonce, as the zero nonce will update to the initial key and never be
       // used for any gas estimations of `execute`, the only function estimated
       db.insert_account_storage(self.address, Self::NONCE_STORAGE_SLOT, U256::from(1)).unwrap();

processor/ethereum/router/src/tests/create_address.rs

@@ -56,42 +56,30 @@ async fn test_create_address() {
   // The only meaningful patterns are < 0x80, == 0x80, and then each length greater > 0x80
   // The following covers all three
   let mut nonce = 1u64;
+  let mut gas = None;
   while nonce.checked_add(nonce).is_some() {
+    let input =
+      (abi::CreateAddress::createAddressForSelfCall { nonce: U256::from(nonce) }).abi_encode();
+
+    // Make sure the function works as expected
+    let call =
+      TransactionRequest::default().to(address).input(TransactionInput::new(input.clone().into()));
     assert_eq!(
-      &test
-        .provider
-        .call(
-          &TransactionRequest::default().to(address).input(TransactionInput::new(
-            (abi::CreateAddress::createAddressForSelfCall { nonce: U256::from(nonce) })
-              .abi_encode()
-              .into()
-          ))
-        )
-        .await
-        .unwrap()
-        .as_ref()[12 ..],
+      &test.provider.call(&call).await.unwrap().as_ref()[12 ..],
       address.create(nonce).as_slice(),
     );
+
+    // Check the function is constant-gas
+    let gas_used = test.provider.estimate_gas(&call).await.unwrap();
+    let initial_gas = calculate_initial_tx_gas(SpecId::CANCUN, &input, false, &[], 0).initial_gas;
+    let this_call = gas_used - initial_gas;
+    if gas.is_none() {
+      gas = Some(this_call);
+    }
+    assert_eq!(gas, Some(this_call));
+
     nonce <<= 1;
   }
 
-  let input =
-    (abi::CreateAddress::createAddressForSelfCall { nonce: U256::from(u64::MAX) }).abi_encode();
-  let gas = test
-    .provider
-    .estimate_gas(
-      &TransactionRequest::default().to(address).input(TransactionInput::new(input.clone().into())),
-    )
-    .await
-    .unwrap() -
-    calculate_initial_tx_gas(SpecId::CANCUN, &input, false, &[], 0).initial_gas;
-
-  let keccak256_gas_estimate = |len: u64| 30 + (6 * len.div_ceil(32));
-  let mut bytecode_len = 0;
-  while (keccak256_gas_estimate(bytecode_len) + keccak256_gas_estimate(85)) < gas {
-    bytecode_len += 32;
-  }
-  println!(
-    "Worst-case createAddress gas: {gas}, CREATE2 break-even is bytecode of length {bytecode_len}",
-  );
+  println!("createAddress gas: {}", gas.unwrap());
 }