From 6c3ff43d6a416abce680e77ce81a9f613ba27075 Mon Sep 17 00:00:00 2001 From: derpy-duck <115193320+derpy-duck@users.noreply.github.com> Date: Fri, 13 Oct 2023 00:10:50 -0400 Subject: [PATCH] Auto relayer refund address handling improvement (#3428) --- .../contracts/relayer/libraries/Utils.sol | 22 ++++++ .../WormholeRelayerDelivery.sol | 64 +++++++++++------- .../wormholeRelayer/WormholeRelayerSend.sol | 2 +- .../forge-test/relayer/WormholeRelayer.t.sol | 67 +++++++++++++++++++ 4 files changed, 131 insertions(+), 24 deletions(-) diff --git a/ethereum/contracts/relayer/libraries/Utils.sol b/ethereum/contracts/relayer/libraries/Utils.sol index 7744ced84..d714d78b9 100644 --- a/ethereum/contracts/relayer/libraries/Utils.sol +++ b/ethereum/contracts/relayer/libraries/Utils.sol @@ -16,6 +16,16 @@ function pay(address payable receiver, LocalNative amount) returns (bool success success = true; } +function pay(address payable receiver, LocalNative amount, uint256 gasBound) returns (bool success) { + uint256 amount_ = LocalNative.unwrap(amount); + if (amount_ != 0) + // TODO: we currently ignore the return data. Some users of this function might want to bubble up the return value though. + // Specifying a higher limit than 63/64 of the remaining gas caps it at that amount without throwing an exception. + (success,) = returnLengthBoundedCall(receiver, new bytes(0), gasBound, amount_, 0); + else + success = true; +} + function min(uint256 a, uint256 b) pure returns (uint256) { return a < b ? a : b; } @@ -47,6 +57,18 @@ uint256 constant freeMemoryPtr = 0x40; uint256 constant memoryWord = 32; uint256 constant maskModulo32 = 0x1f; +/** + * Overload with no 'value' and non-payable address + */ +function returnLengthBoundedCall( + address callee, + bytes memory callData, + uint256 gasLimit, + uint256 dataLengthBound +) returns (bool success, bytes memory returnedData) { + return returnLengthBoundedCall(payable(callee), callData, gasLimit, 0, dataLengthBound); +} + /** * Implements call that truncates return data to a specific size to avoid excessive gas consumption for relayers * when a revert or unexpectedly large return value is produced by the call. diff --git a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol index f05a8b3a4..82b19be39 100644 --- a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol +++ b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerDelivery.sol @@ -25,7 +25,7 @@ import { import {IWormholeReceiver} from "../../interfaces/relayer/IWormholeReceiver.sol"; import {IDeliveryProvider} from "../../interfaces/relayer/IDeliveryProviderTyped.sol"; -import {pay, min, toWormholeFormat, fromWormholeFormat, returnLengthBoundedCall} from "../../relayer/libraries/Utils.sol"; +import {pay, pay, min, toWormholeFormat, fromWormholeFormat, returnLengthBoundedCall, returnLengthBoundedCall} from "../../relayer/libraries/Utils.sol"; import { DeliveryInstruction, DeliveryOverride, @@ -43,6 +43,10 @@ import {WormholeRelayerBase} from "./WormholeRelayerBase.sol"; import "../../interfaces/relayer/TypedUnits.sol"; import "../../relayer/libraries/ExecutionParameters.sol"; +uint256 constant QUOTE_LENGTH_BYTES = 32; + +uint256 constant GAS_LIMIT_EXTERNAL_CALL = 100_000; + abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelayerDelivery { using WormholeRelayerSerde for *; using BytesParsing for bytes; @@ -446,49 +450,63 @@ abstract contract WormholeRelayerDelivery is WormholeRelayerBase, IWormholeRelay uint16 refundChain, bytes32 refundAddress, LocalNative refundAmount, - bytes32 relayerAddress + bytes32 deliveryProvider ) private returns (RefundStatus) { // User requested refund on this chain if (refundChain == getChainId()) { - return pay(payable(fromWormholeFormat(refundAddress)), refundAmount) + return pay(payable(fromWormholeFormat(refundAddress)), refundAmount, GAS_LIMIT_EXTERNAL_CALL) ? RefundStatus.REFUND_SENT : RefundStatus.REFUND_FAIL; } // User requested refund on a different chain - IDeliveryProvider deliveryProvider = IDeliveryProvider(fromWormholeFormat(relayerAddress)); - // Determine price of an 'empty' delivery // (Note: assumes refund chain is an EVM chain) - LocalNative baseDeliveryPrice; - - try deliveryProvider.quoteDeliveryPrice( - refundChain, - TargetNative.wrap(0), - encodeEvmExecutionParamsV1(getEmptyEvmExecutionParamsV1()) - ) returns (LocalNative quote, bytes memory) { - baseDeliveryPrice = quote; - } catch (bytes memory) { - return RefundStatus.CROSS_CHAIN_REFUND_FAIL_PROVIDER_NOT_SUPPORTED; - } - - // If the refundAmount is not greater than the 'empty delivery price', the refund does not go through - if (refundAmount <= getWormholeMessageFee() + baseDeliveryPrice) { - return RefundStatus.CROSS_CHAIN_REFUND_FAIL_NOT_ENOUGH; + (bool success, LocalNative baseDeliveryPrice) = untrustedBaseDeliveryPrice(fromWormholeFormat(deliveryProvider), refundChain); + + // If the unstrusted call failed, or the refundAmount is not greater than the 'empty delivery price', then the refund does not go through + // Note: We first check 'refundAmount <= baseDeliveryPrice', in case an untrusted delivery provider returns a value that overflows once + // the wormhole message fee is added to it + unchecked { + if (!success || (refundAmount <= baseDeliveryPrice) || (refundAmount <= getWormholeMessageFee() + baseDeliveryPrice)) { + return RefundStatus.CROSS_CHAIN_REFUND_FAIL_NOT_ENOUGH; + } } + return sendCrossChainRefund(refundChain, refundAddress, refundAmount, refundAmount - getWormholeMessageFee() - baseDeliveryPrice, deliveryProvider); + } + + function untrustedBaseDeliveryPrice(address deliveryProvider, uint16 refundChain) internal returns (bool success, LocalNative baseDeliveryPrice) { + (bool externalCallSuccess, bytes memory returnData) = returnLengthBoundedCall( + deliveryProvider, + abi.encodeCall(IDeliveryProvider.quoteDeliveryPrice, (refundChain, TargetNative.wrap(0), encodeEvmExecutionParamsV1(getEmptyEvmExecutionParamsV1()))), + GAS_LIMIT_EXTERNAL_CALL, + QUOTE_LENGTH_BYTES + ); + + if(externalCallSuccess && returnData.length == QUOTE_LENGTH_BYTES) { + baseDeliveryPrice = abi.decode(returnData, (LocalNative)); + success = true; + } else { + success = false; + } + } + + function sendCrossChainRefund(uint16 refundChain, bytes32 refundAddress, LocalNative sendAmount, LocalNative receiveAmount, bytes32 deliveryProvider) internal returns (RefundStatus status) { // Request a 'send' with 'paymentForExtraReceiverValue' equal to the refund minus the 'empty delivery price' - try IWormholeRelayerSend(address(this)).send{value: refundAmount.unwrap()}( + // We limit the gas because we are within a delivery, so thus the trust assumptions on the delivery provider are different + // Normally, in 'send', a revert is no problem; but here, we want to prevent such reverts in this try-catch + try IWormholeRelayerSend(address(this)).send{value: sendAmount.unwrap(), gas: GAS_LIMIT_EXTERNAL_CALL}( refundChain, bytes32(0), bytes(""), TargetNative.wrap(0), - refundAmount - getWormholeMessageFee() - baseDeliveryPrice, + receiveAmount, encodeEvmExecutionParamsV1(getEmptyEvmExecutionParamsV1()), refundChain, refundAddress, - fromWormholeFormat(relayerAddress), + fromWormholeFormat(deliveryProvider), new VaaKey[](0), CONSISTENCY_LEVEL_INSTANT ) returns (uint64) { diff --git a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerSend.sol b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerSend.sol index d7721792c..6f0b7938e 100644 --- a/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerSend.sol +++ b/ethereum/contracts/relayer/wormholeRelayer/WormholeRelayerSend.sol @@ -541,7 +541,7 @@ abstract contract WormholeRelayerSend is WormholeRelayerBase, IWormholeRelayerSe bytes32 targetAddress, bytes memory payload, TargetNative receiverValue, - LocalNative paymentForExtraReceiverValue, + LocalNative, bytes memory encodedExecutionParameters, uint16 refundChain, bytes32 refundAddress, diff --git a/ethereum/forge-test/relayer/WormholeRelayer.t.sol b/ethereum/forge-test/relayer/WormholeRelayer.t.sol index a43a745b5..34bdfa60d 100644 --- a/ethereum/forge-test/relayer/WormholeRelayer.t.sol +++ b/ethereum/forge-test/relayer/WormholeRelayer.t.sol @@ -2213,4 +2213,71 @@ contract WormholeRelayerTests is Test { keccak256(setup.target.integration.getMessage()) == keccak256(message), "payload wrong" ); } + + function testProviderRefundAddressZeros( + GasParameters memory gasParams, + FeeParameters memory feeParams, + bytes memory message + ) public { + StandardSetupTwoChains memory setup = standardAssumeAndSetupTwoChains( + gasParams, + feeParams, + 1000000 + ); + vm.recordLogs(); + setup.source.deliveryProvider.updateTargetChainAddress( + setup.targetChain, + bytes32(0x0) + ); + (LocalNative deliveryCost, ) = setup + .source + .coreRelayer + .quoteEVMDeliveryPrice( + setup.targetChain, + TargetNative.wrap(0), + Gas.wrap(gasParams.targetGasLimit) + ); + setup.source.integration.sendMessageWithRefund{ + value: LocalNative.unwrap(deliveryCost) + }( + message, + setup.targetChain, + gasParams.targetGasLimit, + 0, + setup.sourceChain, + address(this) + ); + genericRelayer.relay(setup.sourceChain); + } + + function testSendTargetAddressZeros( + GasParameters memory gasParams, + FeeParameters memory feeParams, + bytes memory message + ) public { + StandardSetupTwoChains memory setup = standardAssumeAndSetupTwoChains( + gasParams, + feeParams, + 1000000 + ); + vm.recordLogs(); + (LocalNative deliveryCost, ) = setup + .source + .coreRelayer + .quoteEVMDeliveryPrice( + setup.targetChain, + TargetNative.wrap(25), + Gas.wrap(gasParams.targetGasLimit) + ); + setup.source.coreRelayer.sendPayloadToEvm{ + value: LocalNative.unwrap(deliveryCost) + }( + setup.targetChain, + address(0x1234123412341234123412341234123412341234), + message, + TargetNative.wrap(25), + Gas.wrap(gasParams.targetGasLimit) + ); + genericRelayer.relay(setup.sourceChain); + } }