From cf4123b0f85c3d9ec5016a3b6cd503a0a2aa76e2 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 2 Nov 2024 10:47:09 -0400 Subject: [PATCH] Update how signatures are handled by the Router --- .../ethereum/router/contracts/Router.sol | 175 +++++++++++------- processor/ethereum/router/src/lib.rs | 33 +++- processor/ethereum/router/src/tests/mod.rs | 20 +- 3 files changed, 151 insertions(+), 77 deletions(-) diff --git a/processor/ethereum/router/contracts/Router.sol b/processor/ethereum/router/contracts/Router.sol index 8607e732..4ec4eb4e 100644 --- a/processor/ethereum/router/contracts/Router.sol +++ b/processor/ethereum/router/contracts/Router.sol @@ -126,76 +126,133 @@ contract Router { /// @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 - * `_nextNonce` - */ + /// @dev Updates the Serai key. This does not update `_nextNonce` /// @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 + function _updateSeraiKey(uint256 nonceUpdatedWith, bytes32 newSeraiKey) private { _seraiKey = newSeraiKey; emit SeraiKeyUpdated(nonceUpdatedWith, newSeraiKey); } /// @notice The constructor for the relayer /// @param initialSeraiKey The initial key for Serai's Ethereum validators - constructor(bytes32 initialSeraiKey) updateSeraiKeyAtEndOfFn(0, initialSeraiKey) { + constructor(bytes32 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 + // Set the Serai key + _updateSeraiKey(0, initialSeraiKey); + + // We just consumed nonce 0 when setting the initial Serai key _nextNonce = 1; // We haven't escaped to any address yet _escapedTo = address(0); } - /// @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 { + /** + * @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. + */ + function verifySignature() + private + returns (uint256 nonceUsed, bytes memory message, bytes32 messageHash) + { // 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)) { + + message = msg.data; + uint256 messageLen = message.length; + /* + function selector, signature + + This check means we don't read memory, and as we attempt to clear portions, write past it + (triggering undefined behavior). + */ + if (messageLen < 68) { revert InvalidSignature(); } + + // Read _nextNonce into memory as the nonce we'll use + nonceUsed = _nextNonce; + + // Declare memory to copy the signature out to + bytes32 signatureC; + bytes32 signatureS; + + // slither-disable-next-line assembly + assembly { + // Read the signature (placed after the function signature) + signatureC := mload(add(message, 36)) + signatureS := mload(add(message, 68)) + + // Overwrite the signature challenge with the nonce + mstore(add(message, 36), nonceUsed) + // Overwrite the signature response with 0 + mstore(add(message, 68), 0) + + // Calculate the message hash + messageHash := keccak256(add(message, 32), messageLen) + } + + // Verify the signature + if (!Schnorr.verify(_seraiKey, messageHash, signatureC, signatureS)) { + revert InvalidSignature(); + } + // Set the next nonce unchecked { - _nextNonce++; + _nextNonce = nonceUsed + 1; + } + + /* + Advance the message past the function selector, enabling decoding the arguments. Ideally, we'd + also advance past the signature (to simplify decoding arguments and save some memory). This + would transfrom message from: + + message (pointer) + v + ------------------------------------------------------------ + | 32-byte length | 4-byte selector | Signature | Arguments | + ------------------------------------------------------------ + + to: + + message (pointer) + v + ---------------------------------------------- + | Junk 68 bytes | 32-byte length | Arguments | + ---------------------------------------------- + + Unfortunately, doing so corrupts the offsets defined within the ABI itself. We settle for a + transform to: + + message (pointer) + v + --------------------------------------------------------- + | Junk 4 bytes | 32-byte length | Signature | Arguments | + --------------------------------------------------------- + */ + // slither-disable-next-line assembly + assembly { + message := add(message, 4) + mstore(message, sub(messageLen, 4)) } } /// @notice Update the key representing Serai's Ethereum validators /// @dev This assumes the key is correct. No checks on it are performed - /// @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(_nextNonce, 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", _nextNonce, newSeraiKey)); - verifySignature(message, signature); + // @param signature The signature by the current key authorizing this update + // @param newSeraiKey The key to update to + function updateSeraiKey() external { + (uint256 nonceUsed, bytes memory args,) = verifySignature(); + (,, bytes32 newSeraiKey) = abi.decode(args, (bytes32, bytes32, bytes32)); + _updateSeraiKey(nonceUsed, newSeraiKey); } /// @notice Transfer coins into Serai with an instruction @@ -357,26 +414,19 @@ contract Router { * @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 + // @param signature The signature by the current key for Serai's Ethereum validators + // @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 // 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 outs, - Signature calldata signature - ) external { - // Verify the signature - // 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", _nextNonce, coin, fee, outs)); - verifySignature(message, signature); + function execute() external { + (uint256 nonceUsed, bytes memory args, bytes32 message) = verifySignature(); + (,, address coin, uint256 fee, OutInstruction[] memory outs) = + abi.decode(args, (bytes32, bytes32, address, uint256, OutInstruction[])); // TODO: Also include a bit mask here - emit Executed(_nextNonce, message); + emit Executed(nonceUsed, message); /* Since we don't have a re-entrancy guard, it is possible for instructions from later batches to @@ -439,9 +489,14 @@ contract Router { /// @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 { + // @param signature The signature by the current key for Serai's Ethereum validators + // @param escapeTo The address to escape to + function escapeHatch() external { + // Verify the signature + (, bytes memory args,) = verifySignature(); + + (,, address escapeTo) = abi.decode(args, (bytes32, bytes32, address)); + if (escapeTo == address(0)) { revert InvalidEscapeAddress(); } @@ -454,10 +509,6 @@ contract Router { revert EscapeHatchInvoked(); } - // Verify the signature - bytes32 message = keccak256(abi.encodePacked("escapeHatch", _nextNonce, escapeTo)); - verifySignature(message, signature); - _escapedTo = escapeTo; emit EscapeHatch(escapeTo); } diff --git a/processor/ethereum/router/src/lib.rs b/processor/ethereum/router/src/lib.rs index eeee70e7..9e2d880a 100644 --- a/processor/ethereum/router/src/lib.rs +++ b/processor/ethereum/router/src/lib.rs @@ -309,26 +309,36 @@ impl Router { /// Get the message to be signed in order to update the key for Serai. pub fn update_serai_key_message(nonce: u64, key: &PublicKey) -> Vec { - ("updateSeraiKey", U256::try_from(nonce).expect("couldn't convert u64 to u256"), key.eth_repr()) - .abi_encode_packed() + [ + abi::updateSeraiKeyCall::SELECTOR.as_slice(), + &(U256::try_from(nonce).unwrap(), U256::ZERO, key.eth_repr()).abi_encode_params(), + ] + .concat() } /// Construct a transaction to update the key representing Serai. pub fn update_serai_key(&self, public_key: &PublicKey, sig: &Signature) -> TxLegacy { TxLegacy { to: TxKind::Call(self.1), - input: abi::updateSeraiKeyCall::new((public_key.eth_repr().into(), sig.into())) - .abi_encode() - .into(), - gas_limit: 40748 * 120 / 100, + input: [ + abi::updateSeraiKeyCall::SELECTOR.as_slice(), + &(abi::Signature::from(sig), public_key.eth_repr()).abi_encode_params(), + ] + .concat() + .into(), + gas_limit: 40927 * 120 / 100, ..Default::default() } } /// Get the message to be signed in order to execute a series of `OutInstruction`s. pub fn execute_message(nonce: u64, coin: Coin, fee: U256, outs: OutInstructions) -> Vec { - ("execute".to_string(), U256::try_from(nonce).unwrap(), coin.address(), fee, outs.0) - .abi_encode_sequence() + [ + abi::executeCall::SELECTOR.as_slice(), + &(U256::try_from(nonce).unwrap(), U256::ZERO, coin.address(), fee, outs.0) + .abi_encode_params(), + ] + .concat() } /// Construct a transaction to execute a batch of `OutInstruction`s. @@ -336,7 +346,12 @@ impl Router { let outs_len = outs.0.len(); TxLegacy { to: TxKind::Call(self.1), - input: abi::executeCall::new((coin.address(), fee, outs.0, sig.into())).abi_encode().into(), + input: [ + abi::executeCall::SELECTOR.as_slice(), + &(abi::Signature::from(sig), coin.address(), fee, outs.0).abi_encode_params(), + ] + .concat() + .into(), // TODO gas_limit: 100_000 + ((200_000 + 10_000) * u128::try_from(outs_len).unwrap()), ..Default::default() diff --git a/processor/ethereum/router/src/tests/mod.rs b/processor/ethereum/router/src/tests/mod.rs index 317003e8..fcd22ec6 100644 --- a/processor/ethereum/router/src/tests/mod.rs +++ b/processor/ethereum/router/src/tests/mod.rs @@ -10,7 +10,7 @@ use alloy_sol_types::SolCall; use alloy_consensus::TxLegacy; -use alloy_rpc_types_eth::BlockNumberOrTag; +use alloy_rpc_types_eth::{BlockNumberOrTag, TransactionReceipt}; use alloy_simple_request_transport::SimpleRequest; use alloy_rpc_client::ClientBuilder; use alloy_provider::RootProvider; @@ -154,8 +154,16 @@ async fn test_erc20_in_instruction() { todo!("TODO") } -async fn publish_outs(key: (Scalar, PublicKey), nonce: u64, coin: Coin, fee: U256, outs: OutInstructions) -> TransactionReceipt { - let msg = Router::execute_message(nonce, coin, fee, instructions.clone()); +async fn publish_outs( + provider: &RootProvider, + router: &Router, + key: (Scalar, PublicKey), + nonce: u64, + coin: Coin, + fee: U256, + outs: OutInstructions, +) -> TransactionReceipt { + let msg = Router::execute_message(nonce, coin, fee, outs.clone()); let nonce = Scalar::random(&mut OsRng); let c = Signature::challenge(ProjectivePoint::GENERATOR * nonce, &key.1, &msg); @@ -163,10 +171,10 @@ async fn publish_outs(key: (Scalar, PublicKey), nonce: u64, coin: Coin, fee: U25 let sig = Signature::new(c, s).unwrap(); - let mut tx = router.execute(coin, fee, instructions, &sig); + let mut tx = router.execute(coin, fee, outs, &sig); tx.gas_price = 100_000_000_000u128; let tx = ethereum_primitives::deterministically_sign(&tx); - ethereum_test_primitives::publish_tx(&provider, tx).await + ethereum_test_primitives::publish_tx(provider, tx).await } #[tokio::test] @@ -182,7 +190,7 @@ async fn test_eth_address_out_instruction() { ethereum_test_primitives::fund_account(&provider, router.address(), amount).await; let instructions = OutInstructions::from([].as_slice()); - let receipt = publish_outs(key, 1, Coin::Ether, fee, instructions); + let receipt = publish_outs(&provider, &router, key, 1, Coin::Ether, fee, instructions).await; assert!(receipt.status()); println!("empty execute used {} gas:", receipt.gas_used);