From 0750988c748d5c3ee79c82ffd49b8875120de4ec Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:26:58 +0100 Subject: [PATCH 1/7] Add test with the expected revert: - remoteSlashAgent from the same domain --- .../test/suite/manager/BondingManager.t.sol | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/contracts-core/test/suite/manager/BondingManager.t.sol b/packages/contracts-core/test/suite/manager/BondingManager.t.sol index 0295b864d3..0bfad20c23 100644 --- a/packages/contracts-core/test/suite/manager/BondingManager.t.sol +++ b/packages/contracts-core/test/suite/manager/BondingManager.t.sol @@ -9,6 +9,7 @@ import { AgentNotUnstaking, CallerNotSummit, MustBeSynapseDomain, + IncorrectOriginDomain, SlashAgentOptimisticPeriod, SynapseDomainForbidden } from "../../../contracts/libs/Errors.sol"; @@ -248,6 +249,14 @@ contract BondingManagerTest is AgentManagerTest { managerMsgPrank(msgPayload); } + function test_remoteSlashAgent_revert_sameOriginDomain() public { + uint32 proofMaturity = BONDING_OPTIMISTIC_PERIOD; + skip(proofMaturity); + bytes memory msgPayload = managerMsgPayload(DOMAIN_SYNAPSE, remoteSlashAgentCalldata(0, address(0), address(0))); + vm.expectRevert(IncorrectOriginDomain.selector); + managerMsgPrank(msgPayload); + } + function test_completeSlashing_active(uint256 domainId, uint256 agentId, address slasher) public { (uint32 domain, address agent) = getAgent(domainId, agentId); // Initiate slashing From 34a067ab87c6a278fcf9755a4cf9290e62bac969 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:27:48 +0100 Subject: [PATCH 2/7] Fix: check origin domain --- .../contracts-core/contracts/manager/BondingManager.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/contracts-core/contracts/manager/BondingManager.sol b/packages/contracts-core/contracts/manager/BondingManager.sol index 15b0b90339..86cddeee2c 100644 --- a/packages/contracts-core/contracts/manager/BondingManager.sol +++ b/packages/contracts-core/contracts/manager/BondingManager.sol @@ -8,6 +8,7 @@ import { CallerNotDestination, CallerNotSummit, IncorrectAgentDomain, + IncorrectOriginDomain, IndexOutOfRange, MerkleTreeFull, MustBeSynapseDomain, @@ -171,8 +172,9 @@ contract BondingManager is AgentManager, InterfaceBondingManager { // Check that merkle proof is mature enough // TODO: separate constant for slashing optimistic period if (proofMaturity < BONDING_OPTIMISTIC_PERIOD) revert SlashAgentOptimisticPeriod(); - // TODO: do we need to save this? - msgOrigin; + // TODO: do we need to save domain where the agent was slashed? + // Message needs to be sent from the remote chain + if (msgOrigin == localDomain) revert IncorrectOriginDomain(); // Slash agent and notify local AgentSecured contracts _slashAgent(domain, agent, prover); // Magic value to return is selector of the called function From 4527220bff62a3ddb99c5ef0c500c47ef1dca05b Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:42:09 +0100 Subject: [PATCH 3/7] Add test with the expected revert: - `execute` sent from the same domain --- .../test/suite/hubs/ExecutionHub.t.sol | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol b/packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol index 23586654c5..69291d79ad 100644 --- a/packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol +++ b/packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol @@ -7,6 +7,7 @@ import { GasLimitTooLow, GasSuppliedTooLow, IncorrectDestinationDomain, + IncorrectOriginDomain, IncorrectMagicValue, IncorrectSnapshotRoot, MessageOptimisticPeriod, @@ -326,6 +327,21 @@ abstract contract ExecutionHubTest is AgentSecuredTest { verify_messageStatusNone(msgLeaf); } + function test_execute_base_revert_originSameDomain() public { + RawBaseMessage memory rbm; + rbm.sender = rbm.recipient = addressToBytes32(recipient); + RawMessage memory rm; + rm.header.flag = uint8(MessageFlag.Base); + rm.header.origin = localDomain(); + rm.header.destination = localDomain(); + rm.header.nonce = 1; + rm.header.optimisticPeriod = 1 seconds; + rm.body = rbm.formatBaseMessage(); + msgPayload = rm.formatMessage(); + vm.expectRevert(IncorrectOriginDomain.selector); + testedEH().execute(msgPayload, new bytes32[](0), new bytes32[](0), 0, 0); + } + function test_execute_base_revert_reentrancy(Random memory random) public { recipient = address(new ReentrantApp()); // Create some simple data From e55f5f08f5802ecb360aa578a22be9b2705b7f38 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:43:14 +0100 Subject: [PATCH 4/7] Fix: check origin domain in `ExecutionHub` --- packages/contracts-core/contracts/hubs/ExecutionHub.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/contracts-core/contracts/hubs/ExecutionHub.sol b/packages/contracts-core/contracts/hubs/ExecutionHub.sol index d2d8114056..d48cdf3b18 100644 --- a/packages/contracts-core/contracts/hubs/ExecutionHub.sol +++ b/packages/contracts-core/contracts/hubs/ExecutionHub.sol @@ -12,6 +12,7 @@ import { DuplicatedSnapshotRoot, IncorrectDestinationDomain, IncorrectMagicValue, + IncorrectOriginDomain, IncorrectSnapshotRoot, GasLimitTooLow, GasSuppliedTooLow, @@ -121,6 +122,8 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec bytes32 msgLeaf = message.leaf(); // Ensure message was meant for this domain if (header.destination() != localDomain) revert IncorrectDestinationDomain(); + // Ensure message was not sent from this domain + if (header.origin() == localDomain) revert IncorrectOriginDomain(); // Check that message has not been executed before ReceiptData memory rcptData = _receiptData[msgLeaf]; if (rcptData.executor != address(0)) revert AlreadyExecuted(); From cb4398ad65b7d4a8ab953abeed27b08c428302f9 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:54:43 +0100 Subject: [PATCH 5/7] Add tests with the expected reverts: - Sending base message to the same domain - Sending manager message to the same domain --- .../contracts-core/test/suite/Origin.t.sol | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/contracts-core/test/suite/Origin.t.sol b/packages/contracts-core/test/suite/Origin.t.sol index b61f4943d4..cbc6b90477 100644 --- a/packages/contracts-core/test/suite/Origin.t.sol +++ b/packages/contracts-core/test/suite/Origin.t.sol @@ -4,7 +4,12 @@ pragma solidity 0.8.17; import {IAgentSecured} from "../../contracts/interfaces/IAgentSecured.sol"; import {InterfaceGasOracle} from "../../contracts/interfaces/InterfaceGasOracle.sol"; import {IStateHub} from "../../contracts/interfaces/IStateHub.sol"; -import {EthTransferFailed, InsufficientEthBalance, TipsValueTooLow} from "../../contracts/libs/Errors.sol"; +import { + EthTransferFailed, + IncorrectDestinationDomain, + InsufficientEthBalance, + TipsValueTooLow +} from "../../contracts/libs/Errors.sol"; import {SNAPSHOT_MAX_STATES} from "../../contracts/libs/Constants.sol"; import {TipsLib} from "../../contracts/libs/stack/Tips.sol"; @@ -98,6 +103,20 @@ contract OriginTest is AgentSecuredTest { ); } + function test_sendBaseMessage_revert_sameDestination() public { + vm.expectRevert(IncorrectDestinationDomain.selector); + vm.prank(sender); + InterfaceOrigin(origin).sendBaseMessage( + localDomain(), addressToBytes32(recipient), period, request.encodeRequest(), "test content" + ); + } + + function test_sendManagementMessage_revert_sameDestination() public { + vm.expectRevert(IncorrectDestinationDomain.selector); + vm.prank(localAgentManager()); + InterfaceOrigin(origin).sendManagerMessage(localDomain(), period, "test payload"); + } + function test_getMinimumTipsValue( uint32 destination_, uint256 paddedRequest, From 5448b42135983c4ed57ce9cfd804fd5462a0c936 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 19:56:42 +0100 Subject: [PATCH 6/7] Fix: revert when message destination is local domain --- packages/contracts-core/contracts/Origin.sol | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/contracts-core/contracts/Origin.sol b/packages/contracts-core/contracts/Origin.sol index d173a92379..e1cf336a7b 100644 --- a/packages/contracts-core/contracts/Origin.sol +++ b/packages/contracts-core/contracts/Origin.sol @@ -4,7 +4,12 @@ pragma solidity 0.8.17; // ══════════════════════════════ LIBRARY IMPORTS ══════════════════════════════ import {BaseMessageLib} from "./libs/memory/BaseMessage.sol"; import {MAX_CONTENT_BYTES} from "./libs/Constants.sol"; -import {ContentLengthTooBig, EthTransferFailed, InsufficientEthBalance} from "./libs/Errors.sol"; +import { + ContentLengthTooBig, + EthTransferFailed, + IncorrectDestinationDomain, + InsufficientEthBalance +} from "./libs/Errors.sol"; import {GasData, GasDataLib} from "./libs/stack/GasData.sol"; import {MemView, MemViewLib} from "./libs/memory/MemView.sol"; import {Header, MessageLib, MessageFlag} from "./libs/memory/Message.sol"; @@ -35,6 +40,11 @@ contract Origin is StateHub, OriginEvents, InterfaceOrigin { address public immutable gasOracle; + modifier onlyRemoteDestination(uint32 destination) { + if (destination == localDomain) revert IncorrectDestinationDomain(); + _; + } + // ═════════════════════════════════════════ CONSTRUCTOR & INITIALIZER ═════════════════════════════════════════════ // solhint-disable-next-line no-empty-blocks @@ -63,7 +73,7 @@ contract Origin is StateHub, OriginEvents, InterfaceOrigin { uint32 optimisticPeriod, uint256 paddedRequest, bytes memory content - ) external payable returns (uint32 messageNonce, bytes32 messageHash) { + ) external payable onlyRemoteDestination(destination) returns (uint32 messageNonce, bytes32 messageHash) { // Check that content is not too large if (content.length > MAX_CONTENT_BYTES) revert ContentLengthTooBig(); // This will revert if msg.value is lower than value of minimum tips @@ -85,6 +95,7 @@ contract Origin is StateHub, OriginEvents, InterfaceOrigin { function sendManagerMessage(uint32 destination, uint32 optimisticPeriod, bytes memory payload) external onlyAgentManager + onlyRemoteDestination(destination) returns (uint32 messageNonce, bytes32 messageHash) { // AgentManager (checked via modifier) is responsible for constructing the calldata payload correctly. From ed0d6f041c58c56761b4957a29348951b7bc831b Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 18 Oct 2023 20:00:37 +0100 Subject: [PATCH 7/7] Update "will revert" documentation --- .../contracts-core/contracts/interfaces/IExecutionHub.sol | 2 ++ .../contracts/interfaces/InterfaceBondingManager.sol | 1 + .../contracts-core/contracts/interfaces/InterfaceOrigin.sol | 5 +++++ 3 files changed, 8 insertions(+) diff --git a/packages/contracts-core/contracts/interfaces/IExecutionHub.sol b/packages/contracts-core/contracts/interfaces/IExecutionHub.sol index 7b5a84e506..77b89a924c 100644 --- a/packages/contracts-core/contracts/interfaces/IExecutionHub.sol +++ b/packages/contracts-core/contracts/interfaces/IExecutionHub.sol @@ -9,6 +9,8 @@ interface IExecutionHub { * previously submitted to this contract in a form of a signed Attestation. * Proven message is immediately executed by passing its contents to the specified recipient. * @dev Will revert if any of these is true: + * - Message is not meant to be executed on this chain + * - Message was sent from this chain * - Message payload is not properly formatted. * - Snapshot root (reconstructed from message hash and proofs) is unknown * - Snapshot root is known, but was submitted by an inactive Notary diff --git a/packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol b/packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol index 0d912d248e..f168998127 100644 --- a/packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol +++ b/packages/contracts-core/contracts/interfaces/InterfaceBondingManager.sol @@ -59,6 +59,7 @@ interface InterfaceBondingManager { * for the OLD agent status. * Note: as an extra security check this function returns its own selector, so that * Destination could verify that a "remote" function was called when executing a manager message. + * Will revert if `msgOrigin` is equal to contract's local domain. * @param domain Domain where the slashed agent was active * @param agent Address of the slashed Agent * @param prover Address that initially provided fraud proof to remote AgentManager diff --git a/packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol b/packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol index 858002bd27..85cb705b34 100644 --- a/packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol +++ b/packages/contracts-core/contracts/interfaces/InterfaceOrigin.sol @@ -7,6 +7,10 @@ interface InterfaceOrigin { /** * @notice Send a message to the recipient located on destination domain. * @dev Recipient has to conform to IMessageRecipient interface, otherwise message won't be delivered. + * Will revert if any of these is true: + * - `destination` is equal to contract's local domain + * - `content` length is greater than `MAX_CONTENT_BYTES` + * - `msg.value` is lower than value of minimum tips for the given message * @param destination Domain of destination chain * @param recipient Address of recipient on destination chain as bytes32 * @param optimisticPeriod Optimistic period for message execution on destination chain @@ -29,6 +33,7 @@ interface InterfaceOrigin { * Note: (msgOrigin, proofMaturity) security args will be added to payload on the destination chain * so that the AgentManager could verify where the Manager Message came from and how mature is the proof. * Note: function is not payable, as no tips are required for sending a manager message. + * Will revert if `destination` is equal to contract's local domain. * @param destination Domain of destination chain * @param optimisticPeriod Optimistic period for message execution on destination chain * @param payload Payload for calling AgentManager on destination chain (with extra security args)