Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Zellic-3.3 (safety checks missing) #1462

Merged
merged 8 commits into from
Oct 23, 2023
Merged
15 changes: 13 additions & 2 deletions packages/contracts-core/contracts/Origin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions packages/contracts-core/contracts/hubs/ExecutionHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
DuplicatedSnapshotRoot,
IncorrectDestinationDomain,
IncorrectMagicValue,
IncorrectOriginDomain,
IncorrectSnapshotRoot,
GasLimitTooLow,
GasSuppliedTooLow,
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
CallerNotDestination,
CallerNotSummit,
IncorrectAgentDomain,
IncorrectOriginDomain,
IndexOutOfRange,
MerkleTreeFull,
MustBeSynapseDomain,
Expand Down Expand Up @@ -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
Expand Down
21 changes: 20 additions & 1 deletion packages/contracts-core/test/suite/Origin.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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,
Expand Down
16 changes: 16 additions & 0 deletions packages/contracts-core/test/suite/hubs/ExecutionHub.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
GasLimitTooLow,
GasSuppliedTooLow,
IncorrectDestinationDomain,
IncorrectOriginDomain,
IncorrectMagicValue,
IncorrectSnapshotRoot,
MessageOptimisticPeriod,
Expand Down Expand Up @@ -347,6 +348,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
AgentNotUnstaking,
CallerNotSummit,
MustBeSynapseDomain,
IncorrectOriginDomain,
SlashAgentOptimisticPeriod,
SynapseDomainForbidden
} from "../../../contracts/libs/Errors.sol";
Expand Down Expand Up @@ -252,6 +253,14 @@ contract BondingManagerTest is AgentManagerTest {
test_remoteSlashAgent_revert_optimisticPeriodNotOver(BONDING_OPTIMISTIC_PERIOD - 1);
}

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
Expand Down
Loading