From 675e72553474ae3574a4fc89786f1302d4e51717 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 12 Oct 2024 10:00:49 +0530 Subject: [PATCH 1/7] agg hook funds not allowed --- .../hooks/aggregation/StaticAggregationHook.sol | 2 ++ solidity/test/hooks/AggregationHook.t.sol | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index 5146ca2c02..8bd8ef025a 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -32,12 +32,14 @@ contract StaticAggregationHook is AbstractPostDispatchHook { ) internal override { address[] memory _hooks = hooks(message); uint256 count = _hooks.length; + uint256 gasRemaining = msg.value; for (uint256 i = 0; i < count; i++) { uint256 quote = IPostDispatchHook(_hooks[i]).quoteDispatch( metadata, message ); + gasRemaining -= quote; IPostDispatchHook(_hooks[i]).postDispatch{value: quote}( metadata, message diff --git a/solidity/test/hooks/AggregationHook.t.sol b/solidity/test/hooks/AggregationHook.t.sol index a37b20f1a1..fbaff0dcde 100644 --- a/solidity/test/hooks/AggregationHook.t.sol +++ b/solidity/test/hooks/AggregationHook.t.sol @@ -72,6 +72,20 @@ contract AggregationHookTest is Test { hook.postDispatch{value: _msgValue}("", message); } + function testPostDispatch_preventsUsingContractFunds(uint8 _hooks) public { + vm.assume(_hooks > 0); + uint256 fee = PER_HOOK_GAS_AMOUNT; + address[] memory hooksDeployed = deployHooks(_hooks, fee); + + // aggregation hook has left over funds + uint256 additionalFunds = 1 ether; + vm.deal(address(hook), additionalFunds); + + bytes memory message = abi.encodePacked("hello world"); + vm.expectRevert(); // underflow + hook.postDispatch{value: 0}("", message); + } + function testQuoteDispatch(uint8 _hooks) public { uint256 fee = PER_HOOK_GAS_AMOUNT; address[] memory hooksDeployed = deployHooks(_hooks, fee); From f8d83ed18c6216649860fa62d4ca0ba9956fac1f Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 16 Oct 2024 00:11:28 +0530 Subject: [PATCH 2/7] it refunds --- .../aggregation/StaticAggregationHook.sol | 15 +++++++++++++ solidity/test/hooks/AggregationHook.t.sol | 21 +++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index 8bd8ef025a..3e63a6386c 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -13,11 +13,19 @@ pragma solidity >=0.8.0; @@@@@@@@@ @@@@@@@@@ @@@@@@@@@ @@@@@@@@*/ +// ============ Internal Imports ============ +import {StandardHookMetadata} from "../libs/StandardHookMetadata.sol"; import {AbstractPostDispatchHook} from "../libs/AbstractPostDispatchHook.sol"; import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol"; import {MetaProxy} from "../../libs/MetaProxy.sol"; +// ============ External Imports ============ +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + contract StaticAggregationHook is AbstractPostDispatchHook { + using StandardHookMetadata for bytes; + using Address for address payable; + // ============ External functions ============ /// @inheritdoc IPostDispatchHook @@ -45,6 +53,13 @@ contract StaticAggregationHook is AbstractPostDispatchHook { message ); } + + if (gasRemaining > 0) { + address payable refundAddress = payable( + metadata.refundAddress(msg.sender) + ); + refundAddress.sendValue(gasRemaining); + } } /// @inheritdoc AbstractPostDispatchHook diff --git a/solidity/test/hooks/AggregationHook.t.sol b/solidity/test/hooks/AggregationHook.t.sol index fbaff0dcde..d344b3da04 100644 --- a/solidity/test/hooks/AggregationHook.t.sol +++ b/solidity/test/hooks/AggregationHook.t.sol @@ -72,10 +72,25 @@ contract AggregationHookTest is Test { hook.postDispatch{value: _msgValue}("", message); } - function testPostDispatch_preventsUsingContractFunds(uint8 _hooks) public { - vm.assume(_hooks > 0); + function test_postDispatch_refundsExcess(uint8 _hooks) public { uint256 fee = PER_HOOK_GAS_AMOUNT; address[] memory hooksDeployed = deployHooks(_hooks, fee); + uint256 requiredValue = hooksDeployed.length * fee; + uint256 overpaidValue = requiredValue + 1000; + + vm.prank(address(this)); + + uint256 initialBalance = address(this).balance; + + bytes memory message = abi.encodePacked("hello world"); + hook.postDispatch{value: overpaidValue}("", message); + + assertEq(address(hook).balance, 0); + assertEq(address(this).balance, initialBalance - requiredValue); + } + + function testPostDispatch_preventsUsingContractFunds(uint8 _hooks) public { + vm.assume(_hooks > 0); // aggregation hook has left over funds uint256 additionalFunds = 1 ether; @@ -108,4 +123,6 @@ contract AggregationHookTest is Test { deployHooks(1, 0); assertEq(hook.hookType(), uint8(IPostDispatchHook.Types.AGGREGATION)); } + + receive() external payable {} } From 37ad671b8c5fdb3a7d6260516d232c5873c66d2c Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 24 Oct 2024 13:45:46 +0530 Subject: [PATCH 3/7] meaningful revert msg --- .../hooks/aggregation/StaticAggregationHook.sol | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index 3e63a6386c..4fb7e98e7a 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -47,18 +47,21 @@ contract StaticAggregationHook is AbstractPostDispatchHook { message ); - gasRemaining -= quote; + require( + gasRemaining >= quote, + "StaticAggregationHook: Insufficient value" + ); + IPostDispatchHook(_hooks[i]).postDispatch{value: quote}( metadata, message ); + + gasRemaining -= quote; } if (gasRemaining > 0) { - address payable refundAddress = payable( - metadata.refundAddress(msg.sender) - ); - refundAddress.sendValue(gasRemaining); + payable(metadata.refundAddress(msg.sender)).sendValue(gasRemaining); } } From edb30495682f44472c6d85341482cc9c72ab7df0 Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 24 Oct 2024 21:36:11 +0530 Subject: [PATCH 4/7] valueRemaining --- .../aggregation/StaticAggregationHook.sol | 19 +++++++---- solidity/test/hooks/AggregationHook.t.sol | 34 ++++++++++++++++--- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index 4fb7e98e7a..c72f457677 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -15,6 +15,8 @@ pragma solidity >=0.8.0; // ============ Internal Imports ============ import {StandardHookMetadata} from "../libs/StandardHookMetadata.sol"; +import {Message} from "../../libs/Message.sol"; +import {TypeCasts} from "../../libs/TypeCasts.sol"; import {AbstractPostDispatchHook} from "../libs/AbstractPostDispatchHook.sol"; import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol"; import {MetaProxy} from "../../libs/MetaProxy.sol"; @@ -23,6 +25,8 @@ import {MetaProxy} from "../../libs/MetaProxy.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; contract StaticAggregationHook is AbstractPostDispatchHook { + using Message for bytes; + using TypeCasts for bytes32; using StandardHookMetadata for bytes; using Address for address payable; @@ -40,16 +44,15 @@ contract StaticAggregationHook is AbstractPostDispatchHook { ) internal override { address[] memory _hooks = hooks(message); uint256 count = _hooks.length; - uint256 gasRemaining = msg.value; + uint256 valueRemaining = msg.value; for (uint256 i = 0; i < count; i++) { uint256 quote = IPostDispatchHook(_hooks[i]).quoteDispatch( metadata, message ); - require( - gasRemaining >= quote, - "StaticAggregationHook: Insufficient value" + valueRemaining >= quote, + "StaticAggregationHook: insufficient value" ); IPostDispatchHook(_hooks[i]).postDispatch{value: quote}( @@ -57,11 +60,13 @@ contract StaticAggregationHook is AbstractPostDispatchHook { message ); - gasRemaining -= quote; + valueRemaining -= quote; } - if (gasRemaining > 0) { - payable(metadata.refundAddress(msg.sender)).sendValue(gasRemaining); + if (valueRemaining > 0) { + payable(metadata.refundAddress(msg.sender)).sendValue( + valueRemaining + ); } } diff --git a/solidity/test/hooks/AggregationHook.t.sol b/solidity/test/hooks/AggregationHook.t.sol index d344b3da04..bbd403ce33 100644 --- a/solidity/test/hooks/AggregationHook.t.sol +++ b/solidity/test/hooks/AggregationHook.t.sol @@ -3,12 +3,16 @@ pragma solidity ^0.8.13; import {Test} from "forge-std/Test.sol"; +import {Message} from "../../contracts/libs/Message.sol"; +import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; import {StaticAggregationHook} from "../../contracts/hooks/aggregation/StaticAggregationHook.sol"; import {StaticAggregationHookFactory} from "../../contracts/hooks/aggregation/StaticAggregationHookFactory.sol"; import {TestPostDispatchHook} from "../../contracts/test/TestPostDispatchHook.sol"; import {IPostDispatchHook} from "../../contracts/interfaces/hooks/IPostDispatchHook.sol"; contract AggregationHookTest is Test { + using TypeCasts for address; + StaticAggregationHookFactory internal factory; StaticAggregationHook internal hook; @@ -72,7 +76,10 @@ contract AggregationHookTest is Test { hook.postDispatch{value: _msgValue}("", message); } - function test_postDispatch_refundsExcess(uint8 _hooks) public { + function test_postDispatch_refundsExcess( + uint8 _hooks, + bytes calldata body + ) public { uint256 fee = PER_HOOK_GAS_AMOUNT; address[] memory hooksDeployed = deployHooks(_hooks, fee); uint256 requiredValue = hooksDeployed.length * fee; @@ -82,23 +89,40 @@ contract AggregationHookTest is Test { uint256 initialBalance = address(this).balance; - bytes memory message = abi.encodePacked("hello world"); + bytes memory message = Message.formatMessage( + 1, + 0, + 1, + address(this).addressToBytes32(), + 2, + address(this).addressToBytes32(), + body + ); hook.postDispatch{value: overpaidValue}("", message); assertEq(address(hook).balance, 0); assertEq(address(this).balance, initialBalance - requiredValue); } - function testPostDispatch_preventsUsingContractFunds(uint8 _hooks) public { + function testPostDispatch_preventsUsingContractFunds( + uint8 _hooks, + bytes calldata body + ) public { + uint256 fee = PER_HOOK_GAS_AMOUNT; + deployHooks(_hooks, fee); vm.assume(_hooks > 0); - // aggregation hook has left over funds + vm.prank(address(this)); + uint256 additionalFunds = 1 ether; vm.deal(address(hook), additionalFunds); bytes memory message = abi.encodePacked("hello world"); - vm.expectRevert(); // underflow + + vm.expectRevert("StaticAggregationHook: insufficient value"); hook.postDispatch{value: 0}("", message); + + assertEq(address(hook).balance, additionalFunds); } function testQuoteDispatch(uint8 _hooks) public { From 5a52f0b62fb86ff1936e84cb614a1b7c96c413ba Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 24 Oct 2024 21:37:14 +0530 Subject: [PATCH 5/7] docs(changeset): Fixed misuse of aggregation hook funds for relaying messages by making sure msg.value is adequate and refunding if excess. --- .changeset/friendly-owls-stare.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/friendly-owls-stare.md diff --git a/.changeset/friendly-owls-stare.md b/.changeset/friendly-owls-stare.md new file mode 100644 index 0000000000..1d75ff0b26 --- /dev/null +++ b/.changeset/friendly-owls-stare.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': minor +--- + +Fixed misuse of aggregation hook funds for relaying messages by making sure msg.value is adequate and refunding if excess. From 434a75e34cecbf60d797a3e3df6141c6e796ee8f Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 25 Oct 2024 04:13:14 +0530 Subject: [PATCH 6/7] message.sender --- .../contracts/hooks/aggregation/StaticAggregationHook.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index c72f457677..7a2e6dfe69 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -64,9 +64,8 @@ contract StaticAggregationHook is AbstractPostDispatchHook { } if (valueRemaining > 0) { - payable(metadata.refundAddress(msg.sender)).sendValue( - valueRemaining - ); + payable(metadata.refundAddress(message.sender().bytes32ToAddress())) + .sendValue(valueRemaining); } } From 7132f052aedceb3d02561203501222a8d0e9a5b6 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 29 Oct 2024 20:41:54 +0530 Subject: [PATCH 7/7] senderAddress --- .../contracts/hooks/aggregation/StaticAggregationHook.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol index 7a2e6dfe69..a8e6fc7acd 100644 --- a/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol +++ b/solidity/contracts/hooks/aggregation/StaticAggregationHook.sol @@ -64,8 +64,9 @@ contract StaticAggregationHook is AbstractPostDispatchHook { } if (valueRemaining > 0) { - payable(metadata.refundAddress(message.sender().bytes32ToAddress())) - .sendValue(valueRemaining); + payable(metadata.refundAddress(message.senderAddress())).sendValue( + valueRemaining + ); } }