diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b994a3cb..cdaae18d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -90,3 +90,25 @@ jobs: run: | cargo install cargo-machete cargo machete + + slither: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac + - name: Slither + run: | + python3 -m pip install solc-select + solc-select install 0.8.26 + solc-select use 0.8.26 + + python3 -m pip install slither-analyzer + + slither --include-paths ./networks/ethereum/schnorr/contracts/Schnorr.sol + slither --include-paths ./networks/ethereum/schnorr/contracts ./networks/ethereum/schnorr/contracts/tests/Schnorr.sol + slither processor/ethereum/deployer/contracts/Deployer.sol + slither processor/ethereum/erc20/contracts/IERC20.sol + + cp networks/ethereum/schnorr/contracts/Schnorr.sol processor/ethereum/router/contracts/ + cp processor/ethereum/erc20/contracts/IERC20.sol processor/ethereum/router/contracts/ + cd processor/ethereum/router/contracts + slither Router.sol diff --git a/networks/ethereum/schnorr/contracts/Schnorr.sol b/networks/ethereum/schnorr/contracts/Schnorr.sol index 57269c5f..69e8d95c 100644 --- a/networks/ethereum/schnorr/contracts/Schnorr.sol +++ b/networks/ethereum/schnorr/contracts/Schnorr.sol @@ -2,12 +2,17 @@ pragma solidity ^0.8.26; /// @title A library for verifying Schnorr signatures -/// @author Luke Parker +/// @author Luke Parker /// @author Elizabeth Binks /// @notice Verifies a Schnorr signature for a specified public key -/// @dev This contract is not complete. Only certain public keys are compatible -/// @dev See https://github.com/serai-dex/serai/blob/next/networks/ethereum/schnorr/src/tests/premise.rs for implementation details -// TODO: Pin to a specific branch/commit once `next` is merged into `develop` +/** + * @dev This contract is not complete (in the cryptographic sense). Only a subset of potential + * public keys are representable here. + * + * See https://github.com/serai-dex/serai/blob/next/networks/ethereum/schnorr/src/tests/premise.rs + * for implementation details + */ +// TODO: Pin above link to a specific branch/commit once `next` is merged into `develop` library Schnorr { // secp256k1 group order uint256 private constant Q = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141; @@ -18,26 +23,30 @@ library Schnorr { uint8 private constant KEY_PARITY = 27; /// @notice Verifies a Schnorr signature for the specified public key - /// @dev The y-coordinate of the public key is assumed to be even - /// @dev The x-coordinate of the public key is assumed to be less than the order of secp256k1 - /// @dev The challenge is calculated as `keccak256(abi.encodePacked(address(R), public_key, message))` where `R` is the commitment to the Schnorr signature's nonce - /// @param public_key The x-coordinate of the public key + /** + * @dev The y-coordinate of the public key is assumed to be even. The x-coordinate of the public + * key is assumed to be less than the order of secp256k1. + * + * The challenge is calculated as `keccak256(abi.encodePacked(address(R), publicKey, message))` + * where `R` is the commitment to the Schnorr signature's nonce. + */ + /// @param publicKey The x-coordinate of the public key /// @param message The (hash of the) message signed /// @param c The challenge for the Schnorr signature /// @param s The response to the challenge for the Schnorr signature /// @return If the signature is valid - function verify(bytes32 public_key, bytes32 message, bytes32 c, bytes32 s) + function verify(bytes32 publicKey, bytes32 message, bytes32 c, bytes32 s) internal pure returns (bool) { // ecrecover = (m, v, r, s) -> key // We instead pass the following to recover the Schnorr signature's nonce (not a public key) - bytes32 sa = bytes32(Q - mulmod(uint256(s), uint256(public_key), Q)); - bytes32 ca = bytes32(Q - mulmod(uint256(c), uint256(public_key), Q)); + bytes32 sa = bytes32(Q - mulmod(uint256(s), uint256(publicKey), Q)); + bytes32 ca = bytes32(Q - mulmod(uint256(c), uint256(publicKey), Q)); /* - The ecrecover precompile checks `r` and `s` (`public_key` and `ca`) are non-zero, banning the + The ecrecover precompile checks `r` and `s` (`publicKey` and `ca`) are non-zero, banning the two keys with zero for their x-coordinate and zero challenges. Each already only had a negligible probability of occuring (assuming zero x-coordinates are even on-curve in the first place). @@ -45,11 +54,11 @@ library Schnorr { `sa` is not checked to be non-zero yet it does not need to be. The inverse of it is never taken. */ - address R = ecrecover(sa, KEY_PARITY, public_key, ca); + address R = ecrecover(sa, KEY_PARITY, publicKey, ca); // The ecrecover failed if (R == address(0)) return false; // Check the signature is correct by rebuilding the challenge - return c == keccak256(abi.encodePacked(R, public_key, message)); + return c == keccak256(abi.encodePacked(R, publicKey, message)); } } diff --git a/networks/ethereum/schnorr/contracts/tests/Schnorr.sol b/networks/ethereum/schnorr/contracts/tests/Schnorr.sol index 92922f5e..1a19371a 100644 --- a/networks/ethereum/schnorr/contracts/tests/Schnorr.sol +++ b/networks/ethereum/schnorr/contracts/tests/Schnorr.sol @@ -8,19 +8,23 @@ import "../Schnorr.sol"; /// @author Elizabeth Binks contract TestSchnorr { /// @notice Verifies a Schnorr signature for the specified public key - /// @dev The y-coordinate of the public key is assumed to be even - /// @dev The x-coordinate of the public key is assumed to be less than the order of secp256k1 - /// @dev The challenge is calculated as `keccak256(abi.encodePacked(address(R), public_key, message))` where `R` is the commitment to the Schnorr signature's nonce - /// @param public_key The x-coordinate of the public key + /** + * @dev The y-coordinate of the public key is assumed to be even. The x-coordinate of the public + * key is assumed to be less than the order of secp256k1. + * + * The challenge is calculated as `keccak256(abi.encodePacked(address(R), publicKey, message))` + * where `R` is the commitment to the Schnorr signature's nonce. + */ + /// @param publicKey The x-coordinate of the public key /// @param message The (hash of the) message signed /// @param c The challenge for the Schnorr signature /// @param s The response to the challenge for the Schnorr signature /// @return If the signature is valid - function verify(bytes32 public_key, bytes calldata message, bytes32 c, bytes32 s) + function verify(bytes32 publicKey, bytes calldata message, bytes32 c, bytes32 s) external pure returns (bool) { - return Schnorr.verify(public_key, keccak256(message), c, s); + return Schnorr.verify(publicKey, keccak256(message), c, s); } } diff --git a/processor/ethereum/deployer/contracts/Deployer.sol b/processor/ethereum/deployer/contracts/Deployer.sol index a7dac1d3..8382cf21 100644 --- a/processor/ethereum/deployer/contracts/Deployer.sol +++ b/processor/ethereum/deployer/contracts/Deployer.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.26; /* - The expected deployment process of the Router is as follows: + The expected deployment process of Serai's Router is as follows: 1) A transaction deploying Deployer is made. Then, a deterministic signature is created such that an account with an unknown private key is the creator of @@ -32,35 +32,57 @@ pragma solidity ^0.8.26; with Serai verifying the published result. This would introduce a DoS risk in the council not publishing the correct key/not publishing any key. - This design does not work with designs expecting initialization (which may require re-deploying - the same code until the initialization successfully goes through, without being sniped). + This design does not work (well) with contracts expecting initialization due + to only allowing deploying init code once (which assumes contracts are + distinct via their constructors). Such designs are unused by Serai so that is + accepted. */ +/// @title Deployer of contracts for the Serai network +/// @author Luke Parker contract Deployer { + /// @return The deployment for some (hashed) init code mapping(bytes32 => address) public deployments; + /// @notice Raised if the provided init code was already prior deployed error PriorDeployed(); + /// @notice Raised if the deployment fails error DeploymentFailed(); - function deploy(bytes memory init_code) external { + /// @notice Deploy the specified init code with `CREATE` + /// @dev This init code is expected to be unique and not prior deployed + /// @param initCode The init code to pass to `CREATE` + function deploy(bytes memory initCode) external { // Deploy the contract - address created_contract; + address createdContract; + // slither-disable-next-line assembly assembly { - created_contract := create(0, add(init_code, 0x20), mload(init_code)) + createdContract := create(0, add(initCode, 0x20), mload(initCode)) } - if (created_contract == address(0)) { + if (createdContract == address(0)) { revert DeploymentFailed(); } - bytes32 init_code_hash = keccak256(init_code); + bytes32 initCodeHash = keccak256(initCode); - // Check this wasn't prior deployed - // We check this *after* deploymeing (in violation of CEI) to handle re-entrancy related bugs - if (deployments[init_code_hash] != address(0)) { + /* + Check this wasn't prior deployed. + + This is a post-check, not a pre-check (in violation of the CEI pattern). If we used a + pre-check, a deployed contract could re-enter the Deployer to deploy the same contract + multiple times due to the inner call updating state and then the outer call overwriting it. + The post-check causes the outer call to error once the inner call updates state. + + This does mean contract deployment may fail if deployment causes arbitrary execution which + maliciously nests deployment of the being-deployed contract. Such an inner call won't fail, + yet the outer call would. The usage of a re-entrancy guard would call the inner call to fail + while the outer call succeeds. This is considered so edge-case it isn't worth handling. + */ + if (deployments[initCodeHash] != address(0)) { revert PriorDeployed(); } // Write the deployment to storage - deployments[init_code_hash] = created_contract; + deployments[initCodeHash] = createdContract; } } diff --git a/processor/ethereum/deployer/src/lib.rs b/processor/ethereum/deployer/src/lib.rs index 6fa59ee3..ff8174a6 100644 --- a/processor/ethereum/deployer/src/lib.rs +++ b/processor/ethereum/deployer/src/lib.rs @@ -26,8 +26,8 @@ mod abi { /// The Deployer contract for the Serai Router contract. /// -/// This Deployer has a deterministic address, letting it be immediately identified on any -/// compatible chain. It then supports retrieving the Router contract's address (which isn't +/// This Deployer has a deterministic address, letting it be immediately identified on any instance +/// of the EVM. It then supports retrieving the deployed contracts addresses (which aren't /// deterministic) using a single call. #[derive(Clone, Debug)] pub struct Deployer(Arc>); @@ -66,6 +66,8 @@ impl Deployer { } /// Construct a new view of the Deployer. + /// + /// This will return None if the Deployer has yet to be deployed on-chain. pub async fn new( provider: Arc>, ) -> Result, RpcError> { diff --git a/processor/ethereum/router/README.md b/processor/ethereum/router/README.md index b93c3219..efb4d0a4 100644 --- a/processor/ethereum/router/README.md +++ b/processor/ethereum/router/README.md @@ -1 +1,5 @@ # Ethereum Router + +The [Router contract](./contracts/Router.sol) is extensively documented to ensure clarity and +understanding of the design decisions made. Please refer to it for understanding of why/what this +is. diff --git a/processor/ethereum/router/contracts/Router.sol b/processor/ethereum/router/contracts/Router.sol index 9100f59e..2fd0f2e4 100644 --- a/processor/ethereum/router/contracts/Router.sol +++ b/processor/ethereum/router/contracts/Router.sol @@ -5,236 +5,493 @@ import "IERC20.sol"; import "Schnorr.sol"; -// _ is used as a prefix for internal functions and smart-contract-scoped variables -contract Router { - // Nonce is incremented for each command executed, preventing replays - uint256 private _nonce; +/* + The Router directly performs low-level calls in order to fine-tune the gas settings. Since this + contract is meant to relay an entire batch of transactions, the ability to exactly meter + individual transactions is critical. - // The nonce which will be used for the smart contracts we deploy, enabling - // predicting their addresses + We don't check the return values as we don't care if the calls succeeded. We solely care we made + them. If someone configures an external contract in a way which borks, we epxlicitly define that + as their fault and out-of-scope to this contract. + + If an actual invariant within Serai exists, an escape hatch exists to move to a new contract. Any + improperly handled actions can be re-signed and re-executed at that point in time. +*/ +// slither-disable-start low-level-calls,unchecked-lowlevel + +/// @title Serai Router +/// @author Luke Parker +/// @notice Intakes coins for the Serai network and handles relaying batches of transfers out +contract Router { + /** + * @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; - // The current public key, defined as per the Schnorr library + /// @dev A nonce incremented upon an action to prevent replays/out-of-order execution + uint256 private _nonce; + + /** + * @dev The current public key for Serai's Ethereum validator set, in the form the Schnorr library + * expects + */ bytes32 private _seraiKey; + /// @dev The address escaped to + address private _escapedTo; + + /// @title The type of destination + /// @dev A destination is either an address or a blob of code to deploy and call enum DestinationType { Address, Code } - struct AddressDestination { - address destination; - } - + /// @title A code destination + /** + * @dev If transferring an ERC20 to this destination, it will be transferred to the address the + * code will be deployed to. If transferring ETH, it will be transferred with the deployment of + * the code. `code` is deployed with CREATE (calling its constructor). The entire deployment + * (and associated sandboxing) must consume less than `gasLimit` units of gas or it will revert. + */ struct CodeDestination { - uint32 gas_limit; + uint32 gasLimit; bytes code; } + /// @title An instruction to transfer coins out + /// @dev Specifies a destination and amount but not the coin as that's assumed to be contextual struct OutInstruction { DestinationType destinationType; bytes destination; - uint256 value; + uint256 amount; } + /// @title A signature + /// @dev Thin wrapper around `c, s` to simplify the API struct Signature { bytes32 c; bytes32 s; } + /// @notice Emitted when the key for Serai's Ethereum validators is updated + /// @param nonce The nonce consumed to update this key + /// @param key The key updated to event SeraiKeyUpdated(uint256 indexed nonce, bytes32 indexed key); + + /// @notice Emitted when an InInstruction occurs + /// @param from The address which called `inInstruction` and caused this event to be emitted + /// @param coin The coin transferred in + /// @param amount The amount of the coin transferred in + /// @param instruction The Shorthand-encoded InInstruction for Serai to decode and handle event InInstruction( address indexed from, address indexed coin, uint256 amount, bytes instruction ); - event Executed(uint256 indexed nonce, bytes32 indexed message_hash); + /// @notice Emitted when a batch of `OutInstruction`s occurs + /// @param nonce The nonce consumed to execute this batch of transactions + /// @param messageHash The hash of the message signed for the executed batch + event Executed(uint256 indexed nonce, bytes32 indexed messageHash); + + /// @notice Emitted when `escapeHatch` is invoked + /// @param escapeTo The address to escape to + event EscapeHatch(address indexed escapeTo); + + /// @notice Emitted when coins escape through the escape hatch + /// @param coin The coin which escaped + event Escaped(address indexed coin); + + /// @notice The contract has had its escape hatch invoked and won't accept further actions + error EscapeHatchInvoked(); + /// @notice The signature was invalid error InvalidSignature(); - error InvalidAmount(); - error FailedTransfer(); + /// @notice The amount specified didn't match `msg.value` + error AmountMismatchesMsgValue(); + /// @notice The call to an ERC20's `transferFrom` failed + error TransferFromFailed(); - // Update the Serai key at the end of the current function. - modifier _updateSeraiKeyAtEndOfFn(uint256 nonceUpdatedWith, bytes32 newSeraiKey) { - // Run the function itself. + /// @notice An invalid address to escape to was specified. + error InvalidEscapeAddress(); + /// @notice Escaping when escape hatch wasn't invoked. + error EscapeHatchNotInvoked(); + + /** + * @dev Updates the Serai key at the end of the current function. Executing at the end of the + * current function allows verifying a signature with the current key. This does not update + * `_nonce` + */ + /// @param nonceUpdatedWith The nonce used to update the key + /// @param newSeraiKey The key updated to + modifier updateSeraiKeyAtEndOfFn(uint256 nonceUpdatedWith, bytes32 newSeraiKey) { + // Run the function itself _; - // Update the key. + // Update the key _seraiKey = newSeraiKey; emit SeraiKeyUpdated(nonceUpdatedWith, newSeraiKey); } - constructor(bytes32 initialSeraiKey) _updateSeraiKeyAtEndOfFn(0, initialSeraiKey) { - // We consumed nonce 0 when setting the initial Serai key - _nonce = 1; + /// @notice The constructor for the relayer + /// @param initialSeraiKey The initial key for Serai's Ethereum validators + constructor(bytes32 initialSeraiKey) updateSeraiKeyAtEndOfFn(0, initialSeraiKey) { // 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 _smartContractNonce = 1; + + // We consumed nonce 0 when setting the initial Serai key + _nonce = 1; + + // We haven't escaped to any address yet + _escapedTo = address(0); } - // updateSeraiKey validates the given Schnorr signature against the current public key, and if - // successful, updates the contract's public key to the one specified. - function updateSeraiKey(bytes32 newSeraiKey, Signature calldata signature) - external - _updateSeraiKeyAtEndOfFn(_nonce, newSeraiKey) - { - // This DST needs a length prefix as well to prevent DSTs potentially being substrings of each - // other, yet this fine for our very well-defined, limited use - bytes32 message = - keccak256(abi.encodePacked("updateSeraiKey", block.chainid, _nonce, newSeraiKey)); - _nonce++; - + /// @dev Verify a signature + /// @param message The message to pass to the Schnorr contract + /// @param signature The signature by the current key for this message + function verifySignature(bytes32 message, Signature calldata signature) private { + // If the escape hatch was triggered, reject further signatures + if (_escapedTo != address(0)) { + revert EscapeHatchInvoked(); + } + // Verify the signature if (!Schnorr.verify(_seraiKey, message, signature.c, signature.s)) { revert InvalidSignature(); } + // Increment the nonce + unchecked { + _nonce++; + } } + /// @notice Update the key representing Serai's Ethereum validators + /// @param newSeraiKey The key to update to + /// @param signature The signature by the current key authorizing this update + function updateSeraiKey(bytes32 newSeraiKey, Signature calldata signature) + external + updateSeraiKeyAtEndOfFn(_nonce, newSeraiKey) + { + /* + This DST needs a length prefix as well to prevent DSTs potentially being substrings of each + other, yet this is fine for our well-defined, extremely-limited use. + + We don't encode the chain ID as Serai generates independent keys for each integration. If + Ethereum L2s are integrated, and they reuse the Ethereum validator set, we would use the + existing Serai key yet we'd apply an off-chain derivation scheme to bind it to specific + networks. This also lets Serai identify EVMs per however it wants, solving the edge case where + two instances of the EVM share a chain ID for whatever horrific reason. + + This uses encodePacked as all items present here are of fixed length. + */ + bytes32 message = keccak256(abi.encodePacked("updateSeraiKey", _nonce, newSeraiKey)); + verifySignature(message, signature); + } + + /// @notice Transfer coins into Serai with an instruction + /// @param coin The coin to transfer in (address(0) if Ether) + /// @param amount The amount to transfer in (msg.value if Ether) + /** + * @param instruction The Shorthand-encoded InInstruction for Serai to associate with this + * transfer in + */ + // Re-entrancy doesn't bork this function + // slither-disable-next-line reentrancy-events function inInstruction(address coin, uint256 amount, bytes memory instruction) external payable { + // Check the transfer if (coin == address(0)) { - if (amount != msg.value) revert InvalidAmount(); + if (amount != msg.value) revert AmountMismatchesMsgValue(); } else { (bool success, bytes memory res) = address(coin).call( abi.encodeWithSelector(IERC20.transferFrom.selector, msg.sender, address(this), amount) ); - // Require there was nothing returned, which is done by some non-standard tokens, or that the - // ERC20 contract did in fact return true + /* + Require there was nothing returned, which is done by some non-standard tokens, or that the + ERC20 contract did in fact return true + */ bool nonStandardResOrTrue = (res.length == 0) || abi.decode(res, (bool)); - if (!(success && nonStandardResOrTrue)) revert FailedTransfer(); + if (!(success && nonStandardResOrTrue)) revert TransferFromFailed(); } /* - Due to fee-on-transfer tokens, emitting the amount directly is frowned upon. The amount - instructed to be transferred may not actually be the amount transferred. + Due to fee-on-transfer tokens, emitting the amount directly is frowned upon. The amount + instructed to be transferred may not actually be the amount transferred. - If we add nonReentrant to every single function which can effect the balance, we can check the - amount exactly matches. This prevents transfers of less value than expected occurring, at - least, not without an additional transfer to top up the difference (which isn't routed through - this contract and accordingly isn't trying to artificially create events from this contract). + If we add nonReentrant to every single function which can effect the balance, we can check the + amount exactly matches. This prevents transfers of less value than expected occurring, at + least, not without an additional transfer to top up the difference (which isn't routed through + this contract and accordingly isn't trying to artificially create events from this contract). - If we don't add nonReentrant, a transfer can be started, and then a new transfer for the - difference can follow it up (again and again until a rounding error is reached). This contract - would believe all transfers were done in full, despite each only being done in part (except - for the last one). + If we don't add nonReentrant, a transfer can be started, and then a new transfer for the + difference can follow it up (again and again until a rounding error is reached). This contract + would believe all transfers were done in full, despite each only being done in part (except + for the last one). - Given fee-on-transfer tokens aren't intended to be supported, the only token actively planned - to be supported is Dai and it doesn't have any fee-on-transfer logic, and how fee-on-transfer - tokens aren't even able to be supported at this time by the larger Serai network, we simply - classify this entire class of tokens as non-standard implementations which induce undefined - behavior. + Given fee-on-transfer tokens aren't intended to be supported, the only token actively planned + to be supported is Dai and it doesn't have any fee-on-transfer logic, and how fee-on-transfer + tokens aren't even able to be supported at this time by the larger Serai network, we simply + classify this entire class of tokens as non-standard implementations which induce undefined + behavior. - It is the Serai network's 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); } - /* - We on purposely do not check if these calls succeed. A call either succeeded, and there's no - problem, or the call failed due to: - A) An insolvency - B) A malicious receiver - C) A non-standard token - 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, - */ + /// @dev Perform an ERC20 transfer out + /// @param to The address to transfer the coins to + /// @param coin The coin to transfer + /// @param amount The amount of the coin to transfer + function erc20TransferOut(address to, address coin, uint256 amount) private { + /* + The ERC20s integrated are presumed to have a constant gas cost, meaning this can only be + insufficient: - // Perform an ERC20 transfer out - function _erc20TransferOut(address to, address coin, uint256 value) private { - coin.call{ gas: 100_000 }(abi.encodeWithSelector(IERC20.transfer.selector, msg.sender, value)); - } + A) An integrated ERC20 uses more gas than this limit (presumed not to be the case) + B) An integrated ERC20 is upgraded (integrated ERC20s are presumed to not be upgradeable) + C) This has a variable gas cost and the user set a hook on receive which caused this (in + which case, we accept dropping this) + D) The user was blacklisted (in which case, we again accept dropping this) + E) Other extreme edge cases, for which such tokens are assumed to not be integrated + F) Ethereum opcodes are repriced in a sufficiently breaking fashion - // Perform an ETH/ERC20 transfer out - function _transferOut(address to, address coin, uint256 value) private { - if (coin == address(0)) { - // Enough gas to service the transfer and a minimal amount of logic - to.call{ value: value, gas: 5_000 }(""); - } else { - _erc20TransferOut(to, coin, value); + This should be in such excess of the gas requirements of integrated tokens we'll survive + repricing, so long as the repricing doesn't revolutionize EVM gas costs as we know it. In such + a case, Serai would have to migrate to a new smart contract using `escapeHatch`. + */ + uint256 _gas = 100_000; + + bytes memory _calldata = abi.encodeWithSelector(IERC20.transfer.selector, to, amount); + bool _success; + // slither-disable-next-line assembly + assembly { + /* + `coin` is trusted so we can accept the risk of a return bomb here, yet we won't check the + return value anyways so there's no need to spend the gas decoding it. We assume failures + are the fault of the recipient, not us, the sender. We don't want to have such errors block + the queue of transfers to make. + + If there ever was some invariant broken, off-chain actions is presumed to occur to move to a + new smart contract with whatever necessary changes made/response occurring. + */ + _success := + call( + _gas, + coin, + // Ether value + 0, + // calldata + add(_calldata, 0x20), + mload(_calldata), + // return data + 0, + 0 + ) } } - /* - Serai supports arbitrary calls out via deploying smart contracts (with user-specified code), - letting them execute whatever calls they're coded for. Since we can't meter CREATE, we call - CREATE from this function which we call not internally, but with CALL (which we can meter). - */ - function arbitaryCallOut(bytes memory code) external payable { + /// @dev Perform an ETH/ERC20 transfer out + /// @param to The address to transfer the coins to + /// @param coin The coin to transfer (address(0) if Ether) + /// @param amount The amount of the coin to transfer + function transferOut(address to, address coin, uint256 amount) private { + if (coin == address(0)) { + // Enough gas to service the transfer and a minimal amount of logic + uint256 _gas = 5_000; + // This uses assembly to prevent return bombs + bool _success; + // slither-disable-next-line assembly + assembly { + _success := + call( + _gas, + to, + amount, + // calldata + 0, + 0, + // return data + 0, + 0 + ) + } + } else { + erc20TransferOut(to, coin, amount); + } + } + + /// @notice Execute some arbitrary code within a secure sandbox + /** + * @dev This performs sandboxing by deploying this code with `CREATE`. This is an external + * function as we can't meter `CREATE`/internal functions. We work around this by calling this + * function with `CALL` (which we can meter). This does forward `msg.value` to the newly + * deployed contract. + */ + /// @param code The code to execute + function executeArbitraryCode(bytes memory code) external payable { // Because we're creating a contract, increment our nonce _smartContractNonce += 1; - uint256 msg_value = msg.value; + uint256 msgValue = msg.value; address contractAddress; + // We need to use assembly here because Solidity doesn't expose CREATE + // slither-disable-next-line assembly assembly { - contractAddress := create(msg_value, add(code, 0x20), mload(code)) + contractAddress := create(msgValue, add(code, 0x20), mload(code)) } } - // Execute a list of transactions if they were signed by the current key with the current nonce + /// @notice Execute a batch of `OutInstruction`s + /** + * @dev All `OutInstruction`s in a batch are only for a single coin to simplify handling of the + * fee + */ + /// @param coin The coin all of these `OutInstruction`s are for + /// @param fee The fee to pay (in coin) to the caller for their relaying of this batch + /// @param outs The `OutInstruction`s to act on + /// @param signature The signature by the current key for Serai's Ethereum validators + // Each individual call is explicitly metered to ensure there isn't a DoS here + // slither-disable-next-line calls-loop function execute( address coin, uint256 fee, - OutInstruction[] calldata transactions, + OutInstruction[] calldata outs, Signature calldata signature ) external { // Verify the signature - // We hash the message here as we need the message's hash for the Executed event - // Since we're already going to hash it, hashing it prior to verifying the signature reduces the - // amount of words hashed by its challenge function (reducing our gas costs) - bytes32 message = - keccak256(abi.encode("execute", block.chainid, _nonce, coin, fee, transactions)); - if (!Schnorr.verify(_seraiKey, message, signature.c, signature.s)) { - revert InvalidSignature(); - } + // This uses `encode`, not `encodePacked`, as `outs` is of variable length + // TODO: Use a custom encode in verifySignature here with assembly (benchmarking before/after) + bytes32 message = keccak256(abi.encode("execute", _nonce, coin, fee, outs)); + verifySignature(message, signature); - // Since the signature was verified, perform execution + // _nonce: Also include a bit mask here emit Executed(_nonce, 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++) { + /* + Since we don't have a re-entrancy guard, it is possible for instructions from later batches to + be executed before these instructions. This is deemed fine. We only require later batches be + relayed after earlier batches in order to form backpressure. This means if a batch has a fee + which isn't worth relaying the batch for, so long as later batches are sufficiently + worthwhile, every batch will be relayed. + */ + + // slither-disable-next-line reentrancy-events + for (uint256 i = 0; i < outs.length; i++) { // If the destination is an address, we perform a direct transfer - if (transactions[i].destinationType == DestinationType.Address) { - // This may cause a panic and the contract to become stuck if the destination isn't actually - // 20 bytes. Serai is trusted to not pass a malformed destination - (AddressDestination memory destination) = - abi.decode(transactions[i].destination, (AddressDestination)); - _transferOut(destination.destination, coin, transactions[i].value); + if (outs[i].destinationType == DestinationType.Address) { + /* + This may cause a revert if the destination isn't actually a valid address. Serai is + trusted to not pass a malformed destination, yet if it ever did, it could simply re-sign a + corrected batch using this nonce. + */ + address destination = abi.decode(outs[i].destination, (address)); + transferOut(destination, coin, outs[i].amount); } else { - // Prepare for the transfer - uint256 eth_value = 0; + // Prepare the transfer + uint256 ethValue = 0; if (coin == address(0)) { - // If it's ETH, we transfer the value with the call - eth_value = transactions[i].value; + // If it's ETH, we transfer the amount with the call + ethValue = outs[i].amount; } else { - // If it's an ERC20, we calculate the hash of the will-be contract and transfer to it - // before deployment. This avoids needing to deploy, then call again, offering a few - // optimizations - address nextAddress = - address(uint160(uint256(keccak256(abi.encode(address(this), _smartContractNonce))))); - _erc20TransferOut(nextAddress, coin, transactions[i].value); + /* + If it's an ERC20, we calculate the address of the will-be contract and transfer to it + before deployment. This avoids needing to deploy the contract, then call transfer, then + call the contract again + */ + address nextAddress = address( + uint160(uint256(keccak256(abi.encodePacked(address(this), _smartContractNonce)))) + ); + erc20TransferOut(nextAddress, coin, outs[i].amount); } - // Perform the deployment with the defined gas budget - (CodeDestination memory destination) = - abi.decode(transactions[i].destination, (CodeDestination)); - address(this).call{ gas: destination.gas_limit, value: eth_value }( - abi.encodeWithSelector(Router.arbitaryCallOut.selector, destination.code) + (CodeDestination memory destination) = abi.decode(outs[i].destination, (CodeDestination)); + + /* + Perform the deployment with the defined gas budget. + + We don't care if the following call fails as we don't want to block/retry if it does. + Failures are considered the recipient's fault. We explicitly do not want the surface + area/inefficiency of caching these for later attempted retires. + + We don't have to worry about a return bomb here as this is our own function which doesn't + return any data. + */ + address(this).call{ gas: destination.gasLimit, value: ethValue }( + abi.encodeWithSelector(Router.executeArbitraryCode.selector, destination.code) ); } } - // Transfer to the caller the fee - _transferOut(msg.sender, coin, fee); + // Transfer the fee to the relayer + transferOut(msg.sender, coin, fee); } + /// @notice Escapes to a new smart contract + /// @dev This should be used upon an invariant being reached or new functionality being needed + /// @param escapeTo The address to escape to + /// @param signature The signature by the current key for Serai's Ethereum validators + function escapeHatch(address escapeTo, Signature calldata signature) external { + if (escapeTo == address(0)) { + revert InvalidEscapeAddress(); + } + /* + We want to define the escape hatch so coins here now, and latently received, can be forwarded. + If the last Serai key set could update the escape hatch, they could siphon off latently + received coins without penalty (if they update the escape hatch after unstaking). + */ + if (_escapedTo != address(0)) { + revert EscapeHatchInvoked(); + } + + // Verify the signature + bytes32 message = keccak256(abi.encodePacked("escapeHatch", _nonce, escapeTo)); + verifySignature(message, signature); + + _escapedTo = escapeTo; + emit EscapeHatch(escapeTo); + } + + /// @notice Escape coins after the escape hatch has been invoked + /// @param coin The coin to escape + function escape(address coin) external { + if (_escapedTo == address(0)) { + revert EscapeHatchNotInvoked(); + } + + emit Escaped(coin); + + // Fetch the amount to escape + uint256 amount = address(this).balance; + if (coin != address(0)) { + amount = IERC20(coin).balanceOf(address(this)); + } + + // Perform the transfer + transferOut(_escapedTo, coin, amount); + } + + /// @notice Fetch the next nonce to use by an action published to this contract + /// return The next nonce to use by an action published to this contract function nonce() external view returns (uint256) { return _nonce; } - function smartContractNonce() external view returns (uint256) { - return _smartContractNonce; - } - + /// @notice Fetch the current key for Serai's Ethereum validator set + /// @return The current key for Serai's Ethereum validator set function seraiKey() external view returns (bytes32) { return _seraiKey; } + + /// @notice Fetch the address escaped to + /// @return The address which was escaped to (address(0) if the escape hatch hasn't been invoked) + function escapedTo() external view returns (address) { + return _escapedTo; + } } + +// slither-disable-end low-level-calls,unchecked-lowlevel diff --git a/processor/ethereum/router/src/lib.rs b/processor/ethereum/router/src/lib.rs index 7a7cffd8..6926425a 100644 --- a/processor/ethereum/router/src/lib.rs +++ b/processor/ethereum/router/src/lib.rs @@ -297,10 +297,9 @@ impl Router { } /// Get the message to be signed in order to update the key for Serai. - pub fn update_serai_key_message(chain_id: U256, nonce: u64, key: &PublicKey) -> Vec { + pub fn update_serai_key_message(nonce: u64, key: &PublicKey) -> Vec { ( "updateSeraiKey", - chain_id, U256::try_from(nonce).expect("couldn't convert u64 to u256"), key.eth_repr(), ) @@ -322,13 +321,12 @@ impl Router { /// Get the message to be signed in order to execute a series of `OutInstruction`s. pub fn execute_message( - chain_id: U256, nonce: u64, coin: Coin, fee: U256, outs: OutInstructions, ) -> Vec { - ("execute", chain_id, U256::try_from(nonce).unwrap(), coin.address(), fee, outs.0).abi_encode() + ("execute", U256::try_from(nonce).unwrap(), coin.address(), fee, outs.0).abi_encode() } /// Construct a transaction to execute a batch of `OutInstruction`s.