From afbdf5d5d12612217158d8e11f0c1f01bfc2169a Mon Sep 17 00:00:00 2001 From: Joe Howarth Date: Tue, 5 Sep 2023 12:36:10 -0700 Subject: [PATCH] AutoRelayer: Simplify executeInstruction to internal call (#3352) --- .../interfaces/relayer/IWormholeRelayer.sol | 20 ++--- .../WormholeRelayerDelivery.sol | 84 +++---------------- .../relayer/BigRevertBufferIntegration.sol | 80 +++++++++++++++++- .../forge-test/relayer/WormholeRelayer.t.sol | 51 ----------- 4 files changed, 96 insertions(+), 139 deletions(-) diff --git a/ethereum/contracts/interfaces/relayer/IWormholeRelayer.sol b/ethereum/contracts/interfaces/relayer/IWormholeRelayer.sol index 8c362321cf..a0e04bac9c 100644 --- a/ethereum/contracts/interfaces/relayer/IWormholeRelayer.sol +++ b/ethereum/contracts/interfaces/relayer/IWormholeRelayer.sol @@ -8,7 +8,7 @@ pragma solidity ^0.8.0; * @notice This project allows developers to build cross-chain applications powered by Wormhole without needing to * write and run their own relaying infrastructure * - * We implement the IWormholeRelayer interface that allows users to request a delivery provider to relay a payload (and/or additional VAAs) + * We implement the IWormholeRelayer interface that allows users to request a delivery provider to relay a payload (and/or additional messages) * to a chain and address of their choice. */ @@ -236,8 +236,8 @@ interface IWormholeRelayerSend is IWormholeRelayerBase { * This function must be called with `msg.value` equal to * quoteEVMDeliveryPrice(targetChain, receiverValue, gasLimit, deliveryProviderAddress) + paymentForExtraReceiverValue * - * MessageKeys can specify wormhole messages (VaaKeys) or other types of messages (ex. USDC CCTP attestations). Ensure the selected - * Note: DeliveryProvider supports all the MessageKey.keyType values specified or it will not be delivered! + * Note: MessageKeys can specify wormhole messages (VaaKeys) or other types of messages (ex. USDC CCTP attestations). Ensure the selected + * DeliveryProvider supports all the MessageKey.keyType values specified or it will not be delivered! * * @param targetChain in Wormhole Chain ID format * @param targetAddress address to call on targetChain (that implements IWormholeReceiver) @@ -323,8 +323,8 @@ interface IWormholeRelayerSend is IWormholeRelayerBase { * This function must be called with `msg.value` equal to * quoteDeliveryPrice(targetChain, receiverValue, encodedExecutionParameters, deliveryProviderAddress) + paymentForExtraReceiverValue * - * MessageKeys can specify wormhole messages (VaaKeys) or other types of messages (ex. USDC CCTP attestations). Ensure the selected - * Note: DeliveryProvider supports all the MessageKey.keyType values specified or it will not be delivered! + * Note: MessageKeys can specify wormhole messages (VaaKeys) or other types of messages (ex. USDC CCTP attestations). Ensure the selected + * DeliveryProvider supports all the MessageKey.keyType values specified or it will not be delivered! * * @param targetChain in Wormhole Chain ID format * @param targetAddress address to call on targetChain (that implements IWormholeReceiver), in Wormhole bytes32 format @@ -528,16 +528,11 @@ interface IWormholeRelayerDelivery is IWormholeRelayerBase { * @custom:member gasUsed - The amount of gas that was used to call your target contract * @custom:member status: * - RECEIVER_FAILURE, if the target contract reverts - * - SUCCESS, if the target contract doesn't revert and no forwards were requested - * - FORWARD_REQUEST_FAILURE, if the target contract doesn't revert, forwards were requested, - * but provided/leftover funds were not sufficient to cover them all - * - FORWARD_REQUEST_SUCCESS, if the target contract doesn't revert and all forwards are covered + * - SUCCESS, if the target contract doesn't revert * @custom:member additionalStatusInfo: - * - If status is SUCCESS or FORWARD_REQUEST_SUCCESS, then this is empty. + * - If status is SUCCESS, then this is empty. * - If status is RECEIVER_FAILURE, this is `RETURNDATA_TRUNCATION_THRESHOLD` bytes of the * return data (i.e. potentially truncated revert reason information). - * - If status is FORWARD_REQUEST_FAILURE, this is also the revert data - the reason the forward failed. - * This will be either an encoded Cancelled, DeliveryProviderReverted, or DeliveryProviderPaymentFailed error * @custom:member refundStatus - Result of the refund. REFUND_SUCCESS or REFUND_FAIL are for * refunds where targetChain=refundChain; the others are for targetChain!=refundChain, * where a cross chain refund is necessary, or if the default code path is used where no refund is requested (NO_REFUND_REQUESTED) @@ -629,7 +624,6 @@ error RequesterNotWormholeRelayer(); //When trying to relay a `DeliveryInstruction` to any other chain but the one it was specified for error TargetChainIsNotThisChain(uint16 targetChain); -error ForwardNotSufficientlyFunded(uint256 amountOfFunds, uint256 amountOfFundsNeeded); //When a `DeliveryOverride` contains a gas limit that's less than the original error InvalidOverrideGasLimit(); //When a `DeliveryOverride` contains a receiver value that's less than the original diff --git a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol index a2475510de..43ed5febfe 100644 --- a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol +++ b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol @@ -127,9 +127,6 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay // ------------------------------------------- PRIVATE ------------------------------------------- - error DeliveryProviderReverted(Gas gasUsed); - error DeliveryProviderPaymentFailed(Gas gasUsed); - struct DeliveryVAAInfo { uint16 sourceChain; uint64 sourceSequence; @@ -239,15 +236,7 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay * vaaInfo.gasLimit and vaaInfo.totalReceiverValue, and `encodedVMs` as the input) * * - Calculates how much gas from `vaaInfo.gasLimit` is left - * - If the call succeeded and during execution of `receiveWormholeMessages` there were - * forward(s), and (gas left from vaaInfo.gasLimit) * (vaaInfo.targetChainRefundPerGasUnused) is enough extra funds - * to execute the forward(s), then the forward(s) are executed - * - else: - * revert the delivery to trigger a receiver failure (or forward request failure if - * there were forward(s)) - * refund 'vaaInfo.targetChainRefundPerGasUnused'*(amount of vaaInfo.gasLimit unused) to vaaInfo.deliveryInstruction.refundAddress - * if the call reverted, refund `vaaInfo.totalReceiverValue` to vaaInfo.deliveryInstruction.refundAddress - * - refund anything leftover to the relayer + * - Refund anything leftover to the relayer * * @param vaaInfo struct specifying: * - sourceChain chain id that the delivery originated from @@ -274,14 +263,7 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay return; } - DeliveryResults memory results; - - // Forces external call - // In order to catch reverts - // (Previously needed for reverts, but waiting to remove since this is sensitive code - // and would need a real audit to redeploy) - try - this.executeInstruction( + DeliveryResults memory results = executeInstruction( EvmDeliveryInstruction({ sourceChain: vaaInfo.sourceChain, targetAddress: vaaInfo.deliveryInstruction.targetAddress, @@ -293,32 +275,10 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay deliveryHash: vaaInfo.deliveryVaaHash, signedVaas: vaaInfo.encodedVMs } - )) returns ( - uint8 _status, Gas _gasUsed, bytes memory targetRevertDataTruncated - ) { - results = DeliveryResults( - _gasUsed, - DeliveryStatus(_status), - targetRevertDataTruncated - ); - } - catch (bytes memory revertData) { - // Should never revert, but see above comment - // Decode returned error (into one of these three known types) - // obtaining the gas usage of targetAddress - bool knownError; - Gas gasUsed_; - (gasUsed_, knownError) = tryDecodeExecuteInstructionError(revertData); - results = DeliveryResults( - knownError? gasUsed_ : vaaInfo.gasLimit, - DeliveryStatus.RECEIVER_FAILURE, - revertData - ); - } + )); setDeliveryBlock(results.status, vaaInfo.deliveryVaaHash); - RefundStatus refundStatus = payRefunds( vaaInfo.deliveryInstruction, vaaInfo.relayerRefundAddress, @@ -329,15 +289,9 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay } function executeInstruction(EvmDeliveryInstruction memory evmInstruction) - external - returns (uint8 status, Gas gasUsed, bytes memory targetRevertDataTruncated) + internal + returns (DeliveryResults memory results) { - // despite being external, we only allow ourselves to call this function (via CALL opcode) - // used as a means to retroactively revert the call to the delivery target if the forwards - // can't be funded - if (msg.sender != address(this)) { - revert RequesterNotWormholeRelayer(); - } Gas gasLimit = evmInstruction.gasLimit; bool success; @@ -357,7 +311,7 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay // Calls the `receiveWormholeMessages` endpoint on the contract `evmInstruction.targetAddress` // (with the gas limit and value specified in instruction, and `encodedVMs` as the input) // If it reverts, returns the first 132 bytes of the revert message - (success, targetRevertDataTruncated) = returnLengthBoundedCall( + (success, results.additionalStatusInfo) = returnLengthBoundedCall( deliveryTarget, callData, gasLimit.unwrap(), @@ -368,16 +322,16 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay Gas postGas = Gas.wrap(gasleft()); unchecked { - gasUsed = (preGas - postGas).min(gasLimit); + results.gasUsed = (preGas - postGas).min(gasLimit); } } if (success) { - targetRevertDataTruncated = new bytes(0); - status = uint8(DeliveryStatus.SUCCESS); + results.additionalStatusInfo = new bytes(0); + results.status = DeliveryStatus.SUCCESS; } else { // Call to 'receiveWormholeMessages' on targetAddress reverted - status = uint8(DeliveryStatus.RECEIVER_FAILURE); + results.status = DeliveryStatus.RECEIVER_FAILURE; } } @@ -529,24 +483,6 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay } } - function tryDecodeExecuteInstructionError( - bytes memory revertData - ) private pure returns (Gas gasUsed, bool knownError) { - uint offset = 0; - bytes4 selector; - // Check to see if the following decode can be performed - if(revertData.length < 36) { - return (Gas.wrap(0), false); - } - (selector, offset) = revertData.asBytes4Unchecked(offset); - if(selector == DeliveryProviderReverted.selector || selector == DeliveryProviderPaymentFailed.selector) { - knownError = true; - uint256 _gasUsed; - (_gasUsed, offset) = revertData.asUint256Unchecked(offset); - gasUsed = Gas.wrap(_gasUsed); - } - } - function checkMessageKeysWithMessages( MessageKey[] memory messageKeys, bytes[] memory signedMessages diff --git a/ethereum/forge-test/relayer/BigRevertBufferIntegration.sol b/ethereum/forge-test/relayer/BigRevertBufferIntegration.sol index 414b03423c..bfde735300 100644 --- a/ethereum/forge-test/relayer/BigRevertBufferIntegration.sol +++ b/ethereum/forge-test/relayer/BigRevertBufferIntegration.sol @@ -1,8 +1,19 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.17; -import "../../contracts/interfaces/relayer/IWormholeReceiver.sol"; import "../../contracts/relayer/libraries/BytesParsing.sol"; +import "../../contracts/interfaces/relayer/IWormholeRelayerTyped.sol"; +import { + EvmDeliveryInstruction +} from "../../contracts/relayer/libraries/RelayerInternalStructs.sol"; +import {WormholeRelayerDelivery} from "../../contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol"; +import {WormholeRelayerBase} from "../../contracts/relayer/wormholeRelayer/WormholeRelayerBase.sol"; +import {IWormholeReceiver} from "../../contracts/interfaces/relayer/IWormholeReceiver.sol"; +import {toWormholeFormat, fromWormholeFormat} from "../../contracts/relayer/libraries/Utils.sol"; +import {MockWormhole} from "./MockWormhole.sol"; + +import "forge-std/Test.sol"; +import "forge-std/console.sol"; uint256 constant uint256Length = 32; @@ -37,3 +48,70 @@ contract BigRevertBufferIntegration is IWormholeReceiver { } } } + +contract ExecuteInstructionHarness is WormholeRelayerDelivery { + constructor(address _wormhole) WormholeRelayerBase(_wormhole) {} + + function executeInstruction_harness(EvmDeliveryInstruction memory instruction) + public + returns (DeliveryResults memory results) + { + return executeInstruction(instruction); + } +} + +contract TestBigBuffers is Test { + ExecuteInstructionHarness harness; + + function setUp() public { + // deploy Wormhole + MockWormhole wormhole = new MockWormhole({ + initChainId: 2, + initEvmChainId: block.chainid + }); + harness = new ExecuteInstructionHarness(address(wormhole)); + console.log(address(harness)); + } + + function testExecuteInstructionTruncatesLongRevertBuffers() public { + console.log(address(harness)); + Gas gasLimit = Gas.wrap(500_000); + uint256 sizeRequested = 512; + bytes32 targetIntegration = toWormholeFormat(address(new BigRevertBufferIntegration())); + // We encode 512 as the requested revert buffer length to our test integration contract + bytes memory payload = abi.encode(sizeRequested); + bytes32 userAddress = toWormholeFormat(address(0x8080)); + + WormholeRelayerDelivery.DeliveryResults memory results = harness.executeInstruction_harness( + EvmDeliveryInstruction({ + sourceChain: 6, + targetAddress: targetIntegration, + payload: payload, + gasLimit: gasLimit, + totalReceiverValue: TargetNative.wrap(0), + targetChainRefundPerGasUnused: GasPrice.wrap(0), + senderAddress: userAddress, + deliveryHash: bytes32(0), + signedVaas: new bytes[](0) + }) + ); + + assertTrue(uint8(results.status) == uint8(IWormholeRelayerDelivery.DeliveryStatus.RECEIVER_FAILURE)); + assertTrue(results.gasUsed <= gasLimit); + assertEq( + results.additionalStatusInfo, + abi.encodePacked( + // First word + bytes32(0x000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f), + // Second word + bytes32(0x202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f), + // Third word + bytes32(0x404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f), + // Fourth word + bytes32(0x606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f), + // Four extra bytes + bytes4(0x80818283) + ) + ); + } +} diff --git a/ethereum/forge-test/relayer/WormholeRelayer.t.sol b/ethereum/forge-test/relayer/WormholeRelayer.t.sol index 1c1910a7c7..5870987d8b 100644 --- a/ethereum/forge-test/relayer/WormholeRelayer.t.sol +++ b/ethereum/forge-test/relayer/WormholeRelayer.t.sol @@ -2210,55 +2210,4 @@ contract WormholeRelayerTests is Test { keccak256(setup.target.integration.getMessage()) == keccak256(message), "payload wrong" ); } - - function testExecuteInstructionTruncatesLongRevertBuffers( - GasParameters memory gasParams, - FeeParameters memory feeParams, - uint32 minTargetGasLimit - ) public { - StandardSetupTwoChains memory setup = - standardAssumeAndSetupTwoChains(gasParams, feeParams, minTargetGasLimit); - Gas gasLimit = Gas.wrap(500_000); - uint256 sizeRequested = 512; - bytes32 targetIntegration = toWormholeFormat(address(new BigRevertBufferIntegration())); - // We encode 512 as the requested revert buffer length to our test integration contract - bytes memory payload = abi.encode(sizeRequested); - bytes32 userAddress = toWormholeFormat(address(0x8080)); - - vm.prank(address(setup.target.coreRelayerFull)); - (uint8 status, Gas gasUsed, bytes memory revertData) = setup - .target - .coreRelayerFull - .executeInstruction( - EvmDeliveryInstruction({ - sourceChain: setup.sourceChain, - targetAddress: targetIntegration, - payload: payload, - gasLimit: gasLimit, - totalReceiverValue: TargetNative.wrap(0), - targetChainRefundPerGasUnused: GasPrice.wrap(0), - senderAddress: userAddress, - deliveryHash: bytes32(0), - signedVaas: new bytes[](0) - }) - ); - - assertTrue(status == uint8(IWormholeRelayerDelivery.DeliveryStatus.RECEIVER_FAILURE)); - assertTrue(gasUsed <= gasLimit); - assertEq( - revertData, - abi.encodePacked( - // First word - bytes32(0x000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f), - // Second word - bytes32(0x202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f), - // Third word - bytes32(0x404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f), - // Fourth word - bytes32(0x606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f), - // Four extra bytes - bytes4(0x80818283) - ) - ); - } }