Remove the Sandbox contract

If instead of intaking calls, we intake code, we can deploy a fresh contract
which makes arbitrary calls *without* attempting to build our abstraction
layer over the concept.

This should have the same gas costs, as we still have one contract deployment.
The new contract only has a constructor, so it should have no actual code and
beat the Sandbox in that regard? We do have to call into ourselves to meter the
gas, yet we already had to call into the deployed Sandbox to achieve that.

Also re-defines the OutInstruction to include tokens, implements
OutInstruction-specified gas amounts, bumps the Solidity version, and other
such misc changes.

.github/actions/build-dependencies/action.yml

@@ -42,8 +42,8 @@ runs:
       shell: bash
       run: |
         cargo install svm-rs
-        svm install 0.8.25
+        svm install 0.8.26
-        svm use 0.8.25
+        svm use 0.8.26
 
     # - name: Cache Rust
     #   uses: Swatinem/rust-cache@a95ba195448af2da9b00fb742d14ffaaf3c21f43

networks/ethereum/build-contracts/src/lib.rs

@@ -6,8 +6,12 @@ use std::{path::PathBuf, fs, process::Command};
 
 /// Build contracts from the specified path, outputting the artifacts to the specified path.
 ///
-/// Requires solc 0.8.25.
+/// Requires solc 0.8.26.
-pub fn build(contracts_path: &str, artifacts_path: &str) -> Result<(), String> {
+pub fn build(
+  include_paths: &[&str],
+  contracts_path: &str,
+  artifacts_path: &str,
+) -> Result<(), String> {
   println!("cargo:rerun-if-changed={contracts_path}/*");
   println!("cargo:rerun-if-changed={artifacts_path}/*");
 
@@ -24,20 +28,24 @@ pub fn build(contracts_path: &str, artifacts_path: &str) -> Result<(), String> {
     if let Some(version) = line.strip_prefix("Version: ") {
       let version =
         version.split('+').next().ok_or_else(|| "no value present on line".to_string())?;
-      if version != "0.8.25" {
+      if version != "0.8.26" {
-        Err(format!("version was {version}, 0.8.25 required"))?
+        Err(format!("version was {version}, 0.8.26 required"))?
       }
     }
   }
 
   #[rustfmt::skip]
-  let args = [
+  let mut args = vec![
     "--base-path", ".",
     "-o", artifacts_path, "--overwrite",
     "--bin", "--bin-runtime", "--abi",
     "--via-ir", "--optimize",
     "--no-color",
   ];
+  for include_path in include_paths {
+    args.push("--include-path");
+    args.push(include_path);
+  }
   let mut args = args.into_iter().map(str::to_string).collect::<Vec<_>>();
 
   let mut queue = vec![PathBuf::from(contracts_path)];
@@ -70,17 +78,17 @@ pub fn build(contracts_path: &str, artifacts_path: &str) -> Result<(), String> {
   }
 
   let solc = Command::new("solc")
-    .args(args)
+    .args(args.clone())
     .output()
     .map_err(|_| "couldn't fetch solc output".to_string())?;
   let stderr =
     String::from_utf8(solc.stderr).map_err(|_| "solc stderr wasn't UTF-8".to_string())?;
   if !solc.status.success() {
-    Err(format!("solc didn't successfully execute: {stderr}"))?;
+    Err(format!("solc (`{}`) didn't successfully execute: {stderr}", args.join(" ")))?;
   }
   for line in stderr.lines() {
     if line.contains("Error:") {
-      Err(format!("solc output had error: {stderr}"))?;
+      Err(format!("solc (`{}`) output had error: {stderr}", args.join(" ")))?;
     }
   }
 

networks/ethereum/schnorr/.gitignore

@@ -1 +0,0 @@
-artifacts

networks/ethereum/schnorr/build.rs

@@ -5,5 +5,5 @@ fn main() {
   if !fs::exists(&artifacts_path).unwrap() {
     fs::create_dir(&artifacts_path).unwrap();
   }
-  build_solidity_contracts::build("contracts", &artifacts_path).unwrap();
+  build_solidity_contracts::build(&[], "contracts", &artifacts_path).unwrap();
 }

networks/ethereum/schnorr/contracts/Schnorr.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: AGPL-3.0-only
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 // See https://github.com/noot/schnorr-verify for implementation details
 library Schnorr {

networks/ethereum/schnorr/contracts/tests/Schnorr.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: AGPL-3.0-only
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 import "../Schnorr.sol";
 

orchestration/src/processor.rs

@@ -21,8 +21,8 @@ pub fn processor(
       if coin == "ethereum" {
         r#"
 RUN cargo install svm-rs
-RUN svm install 0.8.25
+RUN svm install 0.8.26
-RUN svm use 0.8.25
+RUN svm use 0.8.26
 "#
       } else {
         ""

processor/ethereum/contracts/Cargo.toml

@@ -17,7 +17,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 workspace = true
 
 [dependencies]
-alloy-sol-types = { version = "0.8", default-features = false }
+alloy-sol-types = { version = "0.8", default-features = false, features = ["json"] }
 
 [build-dependencies]
 build-solidity-contracts = { path = "../../../networks/ethereum/build-contracts" }

processor/ethereum/contracts/build.rs

@@ -1,3 +1,8 @@
 fn main() {
-  build_solidity_contracts::build("contracts", "artifacts").unwrap();
+  build_solidity_contracts::build(
+    &["../../../networks/ethereum/schnorr/contracts"],
+    "contracts",
+    "artifacts",
+  )
+  .unwrap();
 }

processor/ethereum/contracts/contracts/Deployer.sol

@@ -1,5 +1,5 @@
-// SPDX-License-Identifier: AGPLv3
+// SPDX-License-Identifier: AGPL-3.0-only
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 /*
 The expected deployment process of the Router is as follows:

processor/ethereum/contracts/contracts/IERC20.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: CC0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 interface IERC20 {
   event Transfer(address indexed from, address indexed to, uint256 value);

processor/ethereum/contracts/contracts/Router.sol

@@ -1,23 +1,31 @@
-// SPDX-License-Identifier: AGPLv3
+// SPDX-License-Identifier: AGPL-3.0-only
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 import "./IERC20.sol";
 
-import "./Schnorr.sol";
+import "Schnorr.sol";
-import "./Sandbox.sol";
 
+// _ is used as a prefix for internal functions and smart-contract-scoped variables
 contract Router {
-  // Nonce is incremented for each batch of transactions executed/key update
+  // Nonce is incremented for each command executed, preventing replays
-  uint256 public nonce;
+  uint256 private _nonce;
 
-  // Current public key's x-coordinate
+  // The nonce which will be used for the smart contracts we deploy, enabling
-  // This key must always have the parity defined within the Schnorr contract
+  // predicting their addresses
-  bytes32 public seraiKey;
+  uint256 private _smartContractNonce;
+
+  // The current public key, defined as per the Schnorr library
+  bytes32 private _seraiKey;
+
+  enum DestinationType {
+    Address,
+    Code
+  }
 
   struct OutInstruction {
-    address to;
+    DestinationType destinationType;
-    Call[] calls;
+    bytes destination;
-
+    address coin;
     uint256 value;
   }
 
@@ -26,70 +34,42 @@ contract Router {
     bytes32 s;
   }
 
-  event SeraiKeyUpdated(
+  event SeraiKeyUpdated(uint256 indexed nonce, bytes32 indexed key);
-    uint256 indexed nonce,
+  event InInstruction(address indexed from, address indexed coin, uint256 amount, bytes instruction);
-    bytes32 indexed key,
+  event Executed(uint256 indexed nonce, bytes32 indexed batch);
-    Signature signature
-  );
-  event InInstruction(
-    address indexed from,
-    address indexed coin,
-    uint256 amount,
-    bytes instruction
-  );
-  // success is a uint256 representing a bitfield of transaction successes
-  event Executed(
-    uint256 indexed nonce,
-    bytes32 indexed batch,
-    uint256 success,
-    Signature signature
-  );
 
-  // error types
-  error InvalidKey();
   error InvalidSignature();
   error InvalidAmount();
   error FailedTransfer();
-  error TooManyTransactions();
-
-  modifier _updateSeraiKeyAtEndOfFn(
-    uint256 _nonce,
-    bytes32 key,
-    Signature memory sig
-  ) {
-    if (
-      (key == bytes32(0)) ||
-      ((bytes32(uint256(key) % Schnorr.Q)) != key)
-    ) {
-      revert InvalidKey();
-    }
 
+  // Update the Serai key at the end of the current function.
+  modifier _updateSeraiKeyAtEndOfFn(uint256 nonceUpdatedWith, bytes32 newSeraiKey) {
+    // Run the function itself.
     _;
 
-    seraiKey = key;
+    // Update the key.
-    emit SeraiKeyUpdated(_nonce, key, sig);
+    _seraiKey = newSeraiKey;
+    emit SeraiKeyUpdated(nonceUpdatedWith, newSeraiKey);
   }
 
-  constructor(bytes32 _seraiKey) _updateSeraiKeyAtEndOfFn(
+  constructor(bytes32 initialSeraiKey) _updateSeraiKeyAtEndOfFn(0, initialSeraiKey) {
-    0,
+    // We consumed nonce 0 when setting the initial Serai key
-    _seraiKey,
+    _nonce = 1;
-    Signature({ c: bytes32(0), s: bytes32(0) })
+    // Nonces are incremented by 1 upon account creation, prior to any code execution, per EIP-161
-  ) {
+    // This is incompatible with any networks which don't have their nonces start at 0
-    nonce = 1;
+    _smartContractNonce = 1;
   }
 
-  // updateSeraiKey validates the given Schnorr signature against the current
+  // updateSeraiKey validates the given Schnorr signature against the current public key, and if
-  // public key, and if successful, updates the contract's public key to the
+  // successful, updates the contract's public key to the one specified.
-  // given one.
   function updateSeraiKey(
-    bytes32 _seraiKey,
+    bytes32 newSeraiKey,
-    Signature calldata sig
+    Signature calldata signature
-  ) external _updateSeraiKeyAtEndOfFn(nonce, _seraiKey, sig) {
+  ) external _updateSeraiKeyAtEndOfFn(_nonce, newSeraiKey) {
-    bytes memory message =
+    bytes memory message = abi.encodePacked("updateSeraiKey", block.chainid, _nonce, newSeraiKey);
-      abi.encodePacked("updateSeraiKey", block.chainid, nonce, _seraiKey);
+    _nonce++;
-    nonce++;
 
-    if (!Schnorr.verify(seraiKey, message, sig.c, sig.s)) {
+    if (!Schnorr.verify(_seraiKey, message, signature.c, signature.s)) {
       revert InvalidSignature();
     }
   }
@@ -114,109 +94,121 @@ contract Router {
           )
         );
 
-      // Require there was nothing returned, which is done by some non-standard
+      // Require there was nothing returned, which is done by some non-standard tokens, or that the
-      // tokens, or that the ERC20 contract did in fact return true
+      // ERC20 contract did in fact return true
-      bool nonStandardResOrTrue =
+      bool nonStandardResOrTrue = (res.length == 0) || abi.decode(res, (bool));
-        (res.length == 0) || abi.decode(res, (bool));
       if (!(success && nonStandardResOrTrue)) {
         revert FailedTransfer();
       }
     }
 
     /*
-    Due to fee-on-transfer tokens, emitting the amount directly is frowned upon.
+      Due to fee-on-transfer tokens, emitting the amount directly is frowned upon. The amount
-    The amount instructed to transfer may not actually be the amount
+      instructed to be transferred may not actually be the amount transferred.
-    transferred.
 
-    If we add nonReentrant to every single function which can effect the
+      If we add nonReentrant to every single function which can effect the balance, we can check the
-    balance, we can check the amount exactly matches. This prevents transfers of
+      amount exactly matches. This prevents transfers of less value than expected occurring, at
-    less value than expected occurring, at least, not without an additional
+      least, not without an additional transfer to top up the difference (which isn't routed through
-    transfer to top up the difference (which isn't routed through this contract
+      this contract and accordingly isn't trying to artificially create events from this contract).
-    and accordingly isn't trying to artificially create events).
 
-    If we don't add nonReentrant, a transfer can be started, and then a new
+      If we don't add nonReentrant, a transfer can be started, and then a new transfer for the
-    transfer for the difference can follow it up (again and again until a
+      difference can follow it up (again and again until a rounding error is reached). This contract
-    rounding error is reached). This contract would believe all transfers were
+      would believe all transfers were done in full, despite each only being done in part (except
-    done in full, despite each only being done in part (except for the last
+      for the last one).
-    one).
 
-    Given fee-on-transfer tokens aren't intended to be supported, the only
+      Given fee-on-transfer tokens aren't intended to be supported, the only token actively planned
-    token planned to be supported is Dai and it doesn't have any fee-on-transfer
+      to be supported is Dai and it doesn't have any fee-on-transfer logic, and how fee-on-transfer
-    logic, fee-on-transfer tokens aren't even able to be supported at this time,
+      tokens aren't even able to be supported at this time by the larger Serai network, we simply
-    we simply classify this entire class of tokens as non-standard
+      classify this entire class of tokens as non-standard implementations which induce undefined
-    implementations which induce undefined behavior. It is the Serai network's
+      behavior.
-    role not to add support for any non-standard implementations.
+
+      It is the Serai network's role not to add support for any non-standard implementations.
     */
     emit InInstruction(msg.sender, coin, amount, instruction);
   }
 
-  // execute accepts a list of transactions to execute as well as a signature.
+  // Perform a transfer out
-  // if signature verification passes, the given transactions are executed.
+  function _transferOut(address to, address coin, uint256 value) private {
-  // if signature verification fails, this function will revert.
+    /*
-  function execute(
+      We on purposely do not check if these calls succeed. A call either succeeded, and there's no
-    OutInstruction[] calldata transactions,
+      problem, or the call failed due to:
-    Signature calldata sig
+        A) An insolvency
-  ) external {
+        B) A malicious receiver
-    if (transactions.length > 256) {
+        C) A non-standard token
-      revert TooManyTransactions();
+      A is an invariant, B should be dropped, C is something out of the control of this contract.
+      It is again the Serai's network role to not add support for any non-standard tokens,
+    */
+    if (coin == address(0)) {
+      // Enough gas to service the transfer and a minimal amount of logic
+      to.call{ value: value, gas: 5_000 }("");
+    } else {
+      coin.call{ gas: 100_000 }(abi.encodeWithSelector(IERC20.transfer.selector, msg.sender, value));
+    }
   }
 
-    bytes memory message =
+  /*
-      abi.encode("execute", block.chainid, nonce, transactions);
+    Serai supports arbitrary calls out via deploying smart contracts (with user-specified code),
-    uint256 executed_with_nonce = nonce;
+    letting them execute whatever calls they're coded for. Since we can't meter CREATE, we call
-    // This prevents re-entrancy from causing double spends yet does allow
+    CREATE from this function which we call not internally, but with CALL (which we can meter).
-    // out-of-order execution via re-entrancy
+  */
-    nonce++;
+  function arbitaryCallOut(bytes memory code) external {
+    // Because we're creating a contract, increment our nonce
+    _smartContractNonce += 1;
 
-    if (!Schnorr.verify(seraiKey, message, sig.c, sig.s)) {
+    address contractAddress;
+    assembly {
+      contractAddress := create(0, add(code, 0x20), mload(code))
+    }
+  }
+
+  // Execute a list of transactions if they were signed by the current key with the current nonce
+  function execute(OutInstruction[] calldata transactions, Signature calldata signature) external {
+    // Verify the signature
+    bytes memory message = abi.encode("execute", block.chainid, _nonce, transactions);
+    if (!Schnorr.verify(_seraiKey, message, signature.c, signature.s)) {
       revert InvalidSignature();
     }
 
-    uint256 successes;
+    // Since the signature was verified, perform execution
+    emit Executed(_nonce, keccak256(message));
+    // While this is sufficient to prevent replays, it's still technically possible for instructions
+    // from later batches to be executed before these instructions upon re-entrancy
+    _nonce++;
+
     for (uint256 i = 0; i < transactions.length; i++) {
-      bool success;
+      // If the destination is an address, we perform a direct transfer
-
+      if (transactions[i].destinationType == DestinationType.Address) {
-      // If there are no calls, send to `to` the value
+        // This may cause a panic and the contract to become stuck if the destination isn't actually
-      if (transactions[i].calls.length == 0) {
+        // 20 bytes. Serai is trusted to not pass a malformed destination
-        (success, ) = transactions[i].to.call{
+        (address destination) = abi.decode(transactions[i].destination, (address));
-          value: transactions[i].value,
+        _transferOut(destination, transactions[i].coin, transactions[i].value);
-          gas: 5_000
-        }("");
       } else {
-        // If there are calls, ignore `to`. Deploy a new Sandbox and proxy the
+        // The destination is a piece of initcode. We calculate the hash of the will-be contract,
-        // calls through that
+        // transfer to it, and then run the initcode
-        //
+        address nextAddress =
-        // We could use a single sandbox in order to reduce gas costs, yet that
+          address(uint160(uint256(keccak256(abi.encode(address(this), _smartContractNonce)))));
-        // risks one person creating an approval that's hooked before another
+
-        // user's intended action executes, in order to drain their coins
+        // Perform the transfer
-        //
+        _transferOut(nextAddress, transactions[i].coin, transactions[i].value);
-        // While technically, that would be a flaw in the sandboxed flow, this
+
-        // is robust and prevents such flaws from being possible
+        // Perform the calls with a set gas budget
-        //
+        (uint24 gas, bytes memory code) = abi.decode(transactions[i].destination, (uint24, bytes));
-        // We also don't want people to set state via the Sandbox and expect it
+        address(this).call{
-        // future available when anyone else could set a distinct value
+          gas: gas
-        Sandbox sandbox = new Sandbox();
+        }(abi.encodeWithSelector(Router.arbitaryCallOut.selector, code));
-        (success, ) = address(sandbox).call{
+      }
-          value: transactions[i].value,
+    }
-          // TODO: Have the Call specify the gas up front
-          gas: 350_000
-        }(
-          abi.encodeWithSelector(
-            Sandbox.sandbox.selector,
-            transactions[i].calls
-          )
-        );
   }
 
-      assembly {
+  function nonce() external view returns (uint256) {
-        successes := or(successes, shl(i, success))
+    return _nonce;
   }
+
+  function smartContractNonce() external view returns (uint256) {
+    return _smartContractNonce;
   }
-    emit Executed(
+
-      executed_with_nonce,
+  function seraiKey() external view returns (bytes32) {
-      keccak256(message),
+    return _seraiKey;
-      successes,
-      sig
-    );
   }
 }

processor/ethereum/contracts/contracts/Sandbox.sol

@@ -1,48 +0,0 @@
-// SPDX-License-Identifier: AGPLv3
-pragma solidity ^0.8.24;
-
-struct Call {
-  address to;
-  uint256 value;
-  bytes data;
-}
-
-// A minimal sandbox focused on gas efficiency.
-//
-// The first call is executed if any of the calls fail, making it a fallback.
-// All other calls are executed sequentially.
-contract Sandbox {
-  error AlreadyCalled();
-  error CallsFailed();
-
-  function sandbox(Call[] calldata calls) external payable {
-    // Prevent re-entrancy due to this executing arbitrary calls from anyone
-    // and anywhere
-    bool called;
-    assembly { called := tload(0) }
-    if (called) {
-      revert AlreadyCalled();
-    }
-    assembly { tstore(0, 1) }
-
-    // Execute the calls, starting from 1
-    for (uint256 i = 1; i < calls.length; i++) {
-      (bool success, ) =
-        calls[i].to.call{ value: calls[i].value }(calls[i].data);
-
-      // If this call failed, execute the fallback (call 0)
-      if (!success) {
-        (success, ) =
-          calls[0].to.call{ value: address(this).balance }(calls[0].data);
-        // If this call also failed, revert entirely
-        if (!success) {
-          revert CallsFailed();
-        }
-        return;
-      }
-    }
-
-    // We don't clear the re-entrancy guard as this contract should never be
-    // called again, so there's no reason to spend the effort
-  }
-}

processor/ethereum/contracts/contracts/tests/ERC20.sol

@@ -1,5 +1,5 @@
-// SPDX-License-Identifier: AGPLv3
+// SPDX-License-Identifier: AGPL-3.0-only
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.26;
 
 contract TestERC20 {
   event Transfer(address indexed from, address indexed to, uint256 value);

processor/ethereum/contracts/src/lib.rs

@@ -44,5 +44,3 @@ pub mod router {
   pub const BYTECODE: &str = include_str!("../artifacts/Router.bin");
   pub use super::router_container::Router::*;
 }
-
-pub mod tests;

processor/ethereum/contracts/src/tests.rs

@@ -1,13 +0,0 @@
-use alloy_sol_types::sol;
-
-#[rustfmt::skip]
-#[allow(warnings)]
-#[allow(needless_pass_by_value)]
-#[allow(clippy::all)]
-#[allow(clippy::ignored_unit_patterns)]
-#[allow(clippy::redundant_closure_for_method_calls)]
-mod schnorr_container {
-  use super::*;
-  sol!("contracts/tests/Schnorr.sol");
-}
-pub use schnorr_container::TestSchnorr as schnorr;