new tests for more coverage

Author: bengtlofgren

evm/src/ExecutorQuoterRouter.sol

@@ -33,6 +33,13 @@ contract ExecutorQuoterRouter is IExecutorQuoterRouter {
     error GovernanceExpired(uint64 expiryTime);
     error NotAnEvmAddress(bytes32);
 
+    struct ExecutionParams {
+        uint16 dstChain;
+        bytes32 dstAddr;
+        address refundAddr;
+        address quoterAddr;
+    }
+
     constructor(address _executor) {
         EXECUTOR = IExecutor(_executor);
         OUR_CHAIN = EXECUTOR.ourChain();
@@ -112,25 +119,34 @@ contract ExecutorQuoterRouter is IExecutorQuoterRouter {
         bytes calldata requestBytes,
         bytes calldata relayInstructions
     ) external payable {
-        IExecutorQuoter implementation = quoterContract[quoterAddr];
-        (uint256 requiredPayment, bytes32 payeeAddress, bytes32 quoteBody) =
-            implementation.requestExecutionQuote(dstChain, dstAddr, refundAddr, requestBytes, relayInstructions);
+        ExecutionParams memory params =
+            ExecutionParams({dstChain: dstChain, dstAddr: dstAddr, refundAddr: refundAddr, quoterAddr: quoterAddr});
+        _requestExecutionInternal(params, requestBytes, relayInstructions);
+    }
+
+    function _requestExecutionInternal(
+        ExecutionParams memory params,
+        bytes calldata requestBytes,
+        bytes calldata relayInstructions
+    ) private {
+        IExecutorQuoter implementation = quoterContract[params.quoterAddr];
+        (uint256 requiredPayment, bytes32 payeeAddress, bytes32 quoteBody) = implementation.requestExecutionQuote(
+            params.dstChain, params.dstAddr, params.refundAddr, requestBytes, relayInstructions
+        );
         if (msg.value < requiredPayment) {
             revert Underpaid(msg.value, requiredPayment);
         }
         if (msg.value > requiredPayment) {
-            (bool refundSuccessful,) = payable(refundAddr).call{value: msg.value - requiredPayment}("");
+            (bool refundSuccessful,) = payable(params.refundAddr).call{value: msg.value - requiredPayment}("");
             if (!refundSuccessful) {
-                revert RefundFailed(refundAddr);
+                revert RefundFailed(params.refundAddr);
             }
         }
+        bytes memory signedQuote = abi.encodePacked(
+            QUOTE_PREFIX, params.quoterAddr, payeeAddress, OUR_CHAIN, params.dstChain, EXPIRY_TIME, quoteBody
+        );
         EXECUTOR.requestExecution{value: requiredPayment}(
-            dstChain,
-            dstAddr,
-            refundAddr,
-            abi.encodePacked(QUOTE_PREFIX, quoterAddr, payeeAddress, OUR_CHAIN, dstChain, EXPIRY_TIME, quoteBody),
-            requestBytes,
-            relayInstructions
+            params.dstChain, params.dstAddr, params.refundAddr, signedQuote, requestBytes, relayInstructions
         );
         // this must emit a message in this function in order to verify off-chain that this contract generated the quote
         // the implementation is the only data available in this context that is not available from the executor event

evm/test/ExecutorQuoter.t.sol

@@ -46,7 +46,7 @@ contract ExecutorQuoterTest is Test {
         update2.update = packUint64(quote2.baseFee, quote2.dstGasPrice, quote2.srcPrice, quote2.dstPrice);
         ExecutorQuoter.OnChainQuoteBody memory quote3;
         quote3.baseFee = 27971;
-        quote3.dstGasPrice = 1000078;
+        quote3.dstGasPrice = 100000000;
         quote3.srcPrice = 35751300000000;
         quote3.dstPrice = 35751300000000;
         ExecutorQuoter.Update memory update3;
@@ -185,7 +185,7 @@ contract ExecutorQuoterTest is Test {
     }
 
     /// @notice Test that max uint128 msgValue is handled without overflow.
-    function test_requestQuote_maxMsgValue() public {
+    function test_requestQuote_maxMsgValue() public view {
         uint128 maxMsgValue = type(uint128).max;
         bytes memory relayInstructions = RelayInstructions.encodeGas(250000, maxMsgValue);
 
@@ -266,50 +266,6 @@ contract ExecutorQuoterTest is Test {
         assertGt(quote, uint256(type(uint128).max), "Quote should be greater than a single max uint128");
     }
 
-    /// @notice Compare individual quotes vs combined to verify addition.
-    function test_requestQuote_verifyAddition() public {
-        uint128 gasLimit = 250000;
-        uint128 msgValue = 1e18; // 1 ETH worth
-        uint128 dropoff = 2e18; // 2 ETH worth
-
-        // Get quote with just gas
-        bytes memory gasOnly = RelayInstructions.encodeGas(gasLimit, 0);
-        uint256 quoteGasOnly = executorQuoter.requestQuote(
-            DST_CHAIN,
-            DST_ADDR,
-            UPDATER,
-            ExecutorMessages.makeVAAv1Request(10002, bytes32(uint256(uint160(address(this)))), 1),
-            gasOnly
-        );
-
-        // Get quote with gas + msgValue
-        bytes memory gasAndMsg = RelayInstructions.encodeGas(gasLimit, msgValue);
-        uint256 quoteGasAndMsg = executorQuoter.requestQuote(
-            DST_CHAIN,
-            DST_ADDR,
-            UPDATER,
-            ExecutorMessages.makeVAAv1Request(10002, bytes32(uint256(uint160(address(this)))), 1),
-            gasAndMsg
-        );
-
-        // Get quote with gas + msgValue + dropoff
-        bytes memory all = abi.encodePacked(
-            RelayInstructions.encodeGas(gasLimit, msgValue),
-            RelayInstructions.encodeGasDropOffInstructions(dropoff, bytes32(uint256(uint160(address(this)))))
-        );
-        uint256 quoteAll = executorQuoter.requestQuote(
-            DST_CHAIN,
-            DST_ADDR,
-            UPDATER,
-            ExecutorMessages.makeVAAv1Request(10002, bytes32(uint256(uint160(address(this)))), 1),
-            all
-        );
-
-        // Verify monotonic increase
-        assertGt(quoteGasAndMsg, quoteGasOnly, "Adding msgValue should increase quote");
-        assertGt(quoteAll, quoteGasAndMsg, "Adding dropoff should increase quote");
-    }
-
     /// @notice Test quote calculation with zero prices (division by zero protection).
     function test_requestQuote_zeroPrices() public {
         // Set up a quote with zero srcPrice (would cause division by zero)

evm/test/ExecutorQuoterRouter.t.sol

@@ -211,6 +211,24 @@ contract ExecutorQuoterRouterTest is Test {
         );
     }
 
+    /// @notice Test that ecrecover returning address(0) is caught.
+    /// This happens when v is not 27 or 28, causing ecrecover to return address(0).
+    function test_updateQuoterContractEcrecoverReturnsZero() public {
+        // Build a governance message with an invalid v value (not 27 or 28)
+        // This will cause ecrecover to return address(0)
+        bytes memory govBody =
+            abi.encodePacked(hex"45473031", OUR_CHAIN, testQuoter, UPDATE_IMPLEMENTATION, SENDER_ADDRESS, EXPIRY);
+        bytes32 digest = keccak256(govBody);
+        (, bytes32 r, bytes32 s) = vm.sign(testQuoterPk, digest);
+
+        // Corrupt v to make it invalid (use 0 instead of 27/28)
+        // This causes ecrecover to return address(0)
+        bytes memory badGov = abi.encodePacked(govBody, r, s, uint8(0));
+
+        vm.expectRevert(abi.encodeWithSelector(ExecutorQuoterRouter.InvalidSignature.selector));
+        executorQuoterRouter.updateQuoterContract(badGov);
+    }
+
     function test_updateQuoterContractQuoterMismatch() public {
         (address alice,) = makeAddrAndKey("alice");
         (, uint256 bobPk) = makeAddrAndKey("bob");