From 40ad7b60c462e55f2e2646d261fbeb75b92ef7a3 Mon Sep 17 00:00:00 2001 From: Alexander Date: Tue, 14 May 2024 21:33:46 +0200 Subject: [PATCH 1/6] fix: satisfy compiler warnings --- script/BaseMultiChainDeployer.s.sol | 2 +- script/Deploy.sol | 4 ++-- src/apps/polymer/APolymerEscrow.sol | 2 +- src/apps/polymer/vIBCEscrow.sol | 2 +- .../escrowMessage/refundGasTo.t.sol | 2 +- .../processMessage/Reentry.ack.t.sol | 2 +- .../processMessage/Reentry.call.t.sol | 2 +- .../processMessage/RequirePaymentFromRelayer.t.sol | 8 ++++---- test/OnRecvIncentivizedMockEscrow/Timeout.t.sol | 0 test/wormhole/(wh)messages.t.sol | 2 +- test/wormhole/roundtrip.t.sol | 6 +++--- test/wormhole/verifyMessage2.t.sol | 8 ++++---- test/wormhole/verifyMessages.t.sol | 8 ++++---- 13 files changed, 24 insertions(+), 24 deletions(-) delete mode 100644 test/OnRecvIncentivizedMockEscrow/Timeout.t.sol diff --git a/script/BaseMultiChainDeployer.s.sol b/script/BaseMultiChainDeployer.s.sol index 66e6510..a898eb8 100644 --- a/script/BaseMultiChainDeployer.s.sol +++ b/script/BaseMultiChainDeployer.s.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity ^0.8.22; import "forge-std/Script.sol"; diff --git a/script/Deploy.sol b/script/Deploy.sol index 33f9ebd..5ae3a76 100644 --- a/script/Deploy.sol +++ b/script/Deploy.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity ^0.8.22; import "forge-std/Script.sol"; import {stdJson} from "forge-std/StdJson.sol"; @@ -159,7 +159,7 @@ contract DeployGeneralisedIncentives is BaseMultiChainDeployer { } function deployEscrow(string[] memory bridges) forEachBridge(bridges) internal { - address escrow = deployGeneralisedIncentives(incentiveVersion); + deployGeneralisedIncentives(incentiveVersion); } function _deploy(string[] memory bridges) internal { diff --git a/src/apps/polymer/APolymerEscrow.sol b/src/apps/polymer/APolymerEscrow.sol index a2ff078..198bd14 100644 --- a/src/apps/polymer/APolymerEscrow.sol +++ b/src/apps/polymer/APolymerEscrow.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity ^0.8.22; import {IncentivizedMessageEscrow} from "../../IncentivizedMessageEscrow.sol"; import "../../MessagePayload.sol"; diff --git a/src/apps/polymer/vIBCEscrow.sol b/src/apps/polymer/vIBCEscrow.sol index ca4beaf..ffaee8b 100644 --- a/src/apps/polymer/vIBCEscrow.sol +++ b/src/apps/polymer/vIBCEscrow.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity ^0.8.22; import { APolymerEscrow } from "./APolymerEscrow.sol"; import "../../MessagePayload.sol"; diff --git a/test/IncentivizedMessageEscrow/escrowMessage/refundGasTo.t.sol b/test/IncentivizedMessageEscrow/escrowMessage/refundGasTo.t.sol index f7028bc..776e816 100644 --- a/test/IncentivizedMessageEscrow/escrowMessage/refundGasTo.t.sol +++ b/test/IncentivizedMessageEscrow/escrowMessage/refundGasTo.t.sol @@ -9,7 +9,7 @@ contract EscrowInformationTest is TestCommon { IncentiveDescription storage incentive = _INCENTIVE; incentive.refundGasTo = address(0); vm.expectRevert(); - (, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE)}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE)}( bytes32(uint256(0x123123) + uint256(2**255)), _DESTINATION_ADDRESS_THIS, _MESSAGE, diff --git a/test/IncentivizedMessageEscrow/processMessage/Reentry.ack.t.sol b/test/IncentivizedMessageEscrow/processMessage/Reentry.ack.t.sol index 895ddb1..c9694d5 100644 --- a/test/IncentivizedMessageEscrow/processMessage/Reentry.ack.t.sol +++ b/test/IncentivizedMessageEscrow/processMessage/Reentry.ack.t.sol @@ -68,7 +68,7 @@ contract AckReentryTest is TestCommon, ICrossChainReceiver { bool flag; - function receiveAck(bytes32 /* destinationIdentifier */, bytes32 /* messageIdentifier */, bytes calldata acknowledgement) external { + function receiveAck(bytes32 /* destinationIdentifier */, bytes32 /* messageIdentifier */, bytes calldata /* acknowledgement */) external { vm.expectRevert( abi.encodeWithSignature("MessageAlreadyAcked()") ); diff --git a/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol b/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol index 4af7977..62c5305 100644 --- a/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol +++ b/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol @@ -83,7 +83,7 @@ contract CallReentryTest is TestCommon, ICrossChainReceiver { bool flag; // Receive the message and reentry. - function receiveMessage(bytes32 /* sourceIdentifierbytes */, bytes32 /* messageIdentifier */, bytes calldata /* fromApplication */, bytes calldata message) external returns(bytes memory acknowledgement) { + function receiveMessage(bytes32 /* sourceIdentifierbytes */, bytes32 /* messageIdentifier */, bytes calldata /* fromApplication */, bytes calldata /* message */) external returns(bytes memory acknowledgement) { vm.expectRevert( abi.encodeWithSignature("MessageAlreadySpent()") ); diff --git a/test/IncentivizedMessageEscrow/processMessage/RequirePaymentFromRelayer.t.sol b/test/IncentivizedMessageEscrow/processMessage/RequirePaymentFromRelayer.t.sol index 6f5e830..a645b7d 100644 --- a/test/IncentivizedMessageEscrow/processMessage/RequirePaymentFromRelayer.t.sol +++ b/test/IncentivizedMessageEscrow/processMessage/RequirePaymentFromRelayer.t.sol @@ -63,7 +63,7 @@ contract sendPacketPaymentTest is TestCommon { IncentiveDescription storage incentive = _INCENTIVE; vm.recordLogs(); - (, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( bytes32(uint256(0x123123) + uint256(2**255)), _DESTINATION_ADDRESS_THIS, _MESSAGE, @@ -103,7 +103,7 @@ contract sendPacketPaymentTest is TestCommon { IncentiveDescription storage incentive = _INCENTIVE; vm.recordLogs(); - (, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( bytes32(uint256(0x123123) + uint256(2**255)), _DESTINATION_ADDRESS_THIS, _MESSAGE, @@ -132,7 +132,7 @@ contract sendPacketPaymentTest is TestCommon { IncentiveDescription storage incentive = _INCENTIVE; vm.recordLogs(); - (, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( bytes32(uint256(0x123123) + uint256(2**255)), _DESTINATION_ADDRESS_THIS, _MESSAGE, @@ -180,7 +180,7 @@ contract sendPacketPaymentTest is TestCommon { vm.warp(1000000); IncentiveDescription storage incentive = _INCENTIVE; - (, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE) + SEND_MESSAGE_PAYMENT_COST}( bytes32(uint256(0x123123) + uint256(2**255)), _DESTINATION_ADDRESS_THIS, _MESSAGE, diff --git a/test/OnRecvIncentivizedMockEscrow/Timeout.t.sol b/test/OnRecvIncentivizedMockEscrow/Timeout.t.sol deleted file mode 100644 index e69de29..0000000 diff --git a/test/wormhole/(wh)messages.t.sol b/test/wormhole/(wh)messages.t.sol index a30a84d..c40bb94 100644 --- a/test/wormhole/(wh)messages.t.sol +++ b/test/wormhole/(wh)messages.t.sol @@ -1,7 +1,7 @@ // test/Messages.sol // SPDX-License-Identifier: Apache 2 -pragma solidity ^0.8.0; +pragma solidity ^0.8.22; import "../../src/apps/wormhole/external/wormhole/Messages.sol"; import "../../src/apps/wormhole/external/wormhole/Setters.sol"; diff --git a/test/wormhole/roundtrip.t.sol b/test/wormhole/roundtrip.t.sol index 017fb0b..9c8da75 100644 --- a/test/wormhole/roundtrip.t.sol +++ b/test/wormhole/roundtrip.t.sol @@ -1,7 +1,7 @@ // test/Messages.sol // SPDX-License-Identifier: Apache 2 -pragma solidity ^0.8.0; +pragma solidity ^0.8.22; import "../../src/apps/wormhole/external/wormhole/Messages.sol"; import "../../src/apps/wormhole/external/wormhole/Setters.sol"; @@ -100,7 +100,7 @@ contract TestRoundtrip is Test, IMessageEscrowStructs, Bytes65 { messages.storeGuardianSetPub(initialGuardianSet, uint32(0)); } - function makeValidVM(bytes memory payload, uint16 emitterChainid, bytes32 emitterAddress) internal returns(bytes memory validVM) { + function makeValidVM(bytes memory payload, uint16 emitterChainid, bytes32 emitterAddress) view internal returns(bytes memory validVM) { bytes memory presigsVM = abi.encodePacked( uint8(1), // version uint32(0), // guardianSetIndex @@ -132,7 +132,7 @@ contract TestRoundtrip is Test, IMessageEscrowStructs, Bytes65 { IncentiveDescription storage incentive = _INCENTIVE; vm.recordLogs(); - (uint256 gasRefund, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE)}( + escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE)}( _DESTINATION_IDENTIFIER, convertEVMTo65(address(this)), message, diff --git a/test/wormhole/verifyMessage2.t.sol b/test/wormhole/verifyMessage2.t.sol index b9b315b..e159440 100644 --- a/test/wormhole/verifyMessage2.t.sol +++ b/test/wormhole/verifyMessage2.t.sol @@ -1,7 +1,7 @@ // test/Messages.sol // SPDX-License-Identifier: Apache 2 -pragma solidity ^0.8.0; +pragma solidity ^0.8.22; import "../../src/apps/wormhole/external/wormhole/Messages.sol"; import "../../src/apps/wormhole/external/wormhole/Setters.sol"; @@ -129,10 +129,10 @@ contract TestMessagesC2Sigs is Test { bytes memory invalidVM = abi.encodePacked(validVM, uint8(1)); // Confirm that the test VM is valid - (Structs.VM memory parsedInValidVm, bool valid, string memory reason) = messages.parseAndVerifyVM(invalidVM); + (, bool valid, string memory reason) = messages.parseAndVerifyVM(invalidVM); ( - SmallStructs.SmallVM memory smallVM, - bytes memory payload, + , + , bool valid2, string memory reason2 ) = messages2.parseAndVerifyVM(invalidVM); diff --git a/test/wormhole/verifyMessages.t.sol b/test/wormhole/verifyMessages.t.sol index 2dcc5f2..1f252aa 100644 --- a/test/wormhole/verifyMessages.t.sol +++ b/test/wormhole/verifyMessages.t.sol @@ -1,7 +1,7 @@ // test/Messages.sol // SPDX-License-Identifier: Apache 2 -pragma solidity ^0.8.0; +pragma solidity ^0.8.22; import "../../src/apps/wormhole/external/wormhole/Messages.sol"; import "../../src/apps/wormhole/external/wormhole/Setters.sol"; @@ -105,10 +105,10 @@ contract TestMessagesC is Test { bytes memory invalidVM = abi.encodePacked(validVM, uint8(1)); // Confirm that the test VM is valid - (Structs.VM memory parsedInValidVm, bool valid, string memory reason) = messages.parseAndVerifyVM(invalidVM); + (, bool valid, string memory reason) = messages.parseAndVerifyVM(invalidVM); ( - SmallStructs.SmallVM memory smallVM, - bytes memory payload, + , + , bool valid2, string memory reason2 ) = messages2.parseAndVerifyVM(invalidVM); From 8c190a4ab95b0765bfc7e707ab4e9ca73033a5a3 Mon Sep 17 00:00:00 2001 From: Alexander Date: Tue, 14 May 2024 21:33:46 +0200 Subject: [PATCH 2/6] feat: doc changes --- src/IncentivizedMessageEscrow.sol | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol index 2599a46..92c3d4d 100644 --- a/src/IncentivizedMessageEscrow.sol +++ b/src/IncentivizedMessageEscrow.sol @@ -12,19 +12,19 @@ import "./MessagePayload.sol"; /** * @title Generalised Incentive Escrow - * @author Alexander @ Catalyst + * @author Cata labs for Catalyst * @notice Places transparent incentives on relaying messages. * This contract is intended to sit between an application and a cross-chain message protocol. - * The goal is to overload the existing incentive scheme with one which is open for anyone. + * The goal is to overload the existing incentive scheme with one that is open to anyone. * - * Each messaging protocol will have a respective implementation which understands + * Each messaging protocol will have a respective implementation that understands * how to send and verify messages. An integrating application shall deliver a message to submitMessage - * along with the respective incentives. This contract will then handle transfering the message to the - * destination and carry an ack back from the destination to return to the integrating application. + * along with the respective incentives. An integration of this contract will then handle transfering the + * message to the destination and carry an ack back from the destination to return to the integrating application. * * The incentive is released when an ack from the destination chain is delivered to this contract. * - * Beyond making relayer incentives strong, this contract also implements several quality of life features: + * Beyond making relayer incentives stronger, this contract also implements several quality of life features: * - Refund unused gas. * - Seperate gas payments for call and ack. * - Simple implementation of new messaging protocols. @@ -34,13 +34,13 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes //--- Constants ---// /** @notice If a swap reverts on the destination chain, 1 bytes is sent back instead. This is the byte. */ - bytes1 constant public MESSAGE_REVERTED = 0xff; + bytes1 constant MESSAGE_REVERTED = 0xff; /** @notice If the original sender is not authorised on the application on the destination chain, 1 bytes is sent back instead. This is the byte. */ - bytes1 constant public NO_AUTHENTICATION = 0xfe; + bytes1 constant NO_AUTHENTICATION = 0xfe; /** @notice Message timed out on destination chain. */ - bytes1 constant public MESSAGE_TIMED_OUT = 0xfd; + bytes1 constant MESSAGE_TIMED_OUT = 0xfd; /** * @notice If setRemoteImplementation is called with this as the @@ -52,16 +52,16 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice If a relayer or application provides an address which cannot accept gas and the transfer fails * the gas is sent here instead. - * @dev This may not invoke any logic on receive() + * @dev This may not invoke any logic on receive() or be a proxy. */ address immutable public SEND_LOST_GAS_TO; //--- Storage ---// /** - * @notice messageIdentifier to IncentiveDescription. + * @notice Get incentive description based on message context: messageIdentifier, fromApplication, and destChain * @dev fromApplication and destChain are required to fetch the bounty since those * match to exactly 1 remote escrow implementation. As a result, this restricts this storage - * slot's security to the security of the remote escrow implementation. + * slot's security to the security of the specified remote escrow implementation. */ mapping(address fromApplication => mapping(bytes32 destChain => mapping(bytes32 messageIdentifier => IncentiveDescription))) _bounty; @@ -146,7 +146,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes } /** - * @param sendLostGasTo It should only be set to an EOA or a contract which implements either a fallback or a receive function that never reverts. + * @param sendLostGasTo It should only be set to an EOA or a contract that has no logic on receive. It cannot be a proxy. * It cannot be set to address 0, instead use a burn address (0xdead) if no-one wants to take responsibility of the Ether. */ constructor(address sendLostGasTo) { @@ -174,13 +174,13 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice Generates a unique message identifier for a message - * @dev Should be overwritten. The identifier should: + * @dev The identifier should: * - Be unique based on the sender such that applications can't be DoS'ed. * - Be unique over time: Use blocknumber or blockhash * - Be unique on the source chain: Use a unique destinationIdentifier * - Be unique on destination chain: Use a unique source identifier * - Depend on the message - * This also implies that application should make their messages user specific. + * This also implies that application should make their messages user specific, say include a user address. */ function _getMessageIdentifier( address messageSender, From b9408b835189c572c48e6bb66ec87ecc755bcf2d Mon Sep 17 00:00:00 2001 From: Alexander Date: Tue, 14 May 2024 21:33:46 +0200 Subject: [PATCH 3/6] doc: explain _verifyTimeout --- src/IncentivizedMessageEscrow.sol | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol index 92c3d4d..1297309 100644 --- a/src/IncentivizedMessageEscrow.sol +++ b/src/IncentivizedMessageEscrow.sol @@ -777,6 +777,16 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes ); } + /** + * @notice Verifies the input parameters are contained messageIdentfier and that the other arguments are valid. + * The usage of this function is intended when no parameters of a message can be trusted and we have to verify them. + * This is the case when we receive a timeout, as the timeout had to be emitted without any verification + * on the remote chain, for us to verify when the know if a message identifier is good AND how to compute it. + * + * @dev This function uses the fact that hash(a) == hash(b) IFF a == b. So if someone proposes b, we have hash(a) + * then we can check if b == a by hashing b and comparing to a. + * a is the initial state when the message was initiated and b is the proposed state from the timeout. + */ function _verifyTimeout(bytes32 destinationIdentifier, bytes memory implementationIdentifier, bytes calldata message) internal view returns(bytes32 messageIdentifier, address fromApplication, bytes calldata applicationMessage) { // First check if the application trusts the implementation on the destination chain. This is very important // since the remote implementation NEEDS to check that the message hasn't been executed before the deadline From 5ace3f87bb55ac4fd3d29ba1cb5861611c9d6261 Mon Sep 17 00:00:00 2001 From: Alexander Date: Tue, 14 May 2024 22:43:55 +0200 Subject: [PATCH 4/6] feat: improve documentation further --- src/IncentivizedMessageEscrow.sol | 193 +++++++++++++++++------------- 1 file changed, 109 insertions(+), 84 deletions(-) diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol index 1297309..362ab83 100644 --- a/src/IncentivizedMessageEscrow.sol +++ b/src/IncentivizedMessageEscrow.sol @@ -12,15 +12,17 @@ import "./MessagePayload.sol"; /** * @title Generalised Incentive Escrow - * @author Cata labs for Catalyst - * @notice Places transparent incentives on relaying messages. + * @author Cata labs Inc. + * @notice Main logic for placing transparent incentives on message relaying. * This contract is intended to sit between an application and a cross-chain message protocol. * The goal is to overload the existing incentive scheme with one that is open to anyone. * - * Each messaging protocol will have a respective implementation that understands - * how to send and verify messages. An integrating application shall deliver a message to submitMessage - * along with the respective incentives. An integration of this contract will then handle transfering the - * message to the destination and carry an ack back from the destination to return to the integrating application. + * Each messaging protocol will have a respective implementation that understands how to send + * and verify messages. There are 4 functions that an integration has to implement. + * Any implementation of this contract, allows applications to deliver a message to ::submitMessage + * along with the respective incentives. + * The integration (this contract) will handle transfering the message to the destination and + * returning an ack from the destination to the integrating application. * * The incentive is released when an ack from the destination chain is delivered to this contract. * @@ -28,39 +30,52 @@ import "./MessagePayload.sol"; * - Refund unused gas. * - Seperate gas payments for call and ack. * - Simple implementation of new messaging protocols. + * + * Applications integration with Generalised Incentives have to be aware that Acks are replayable. */ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes65 { //--- Constants ---// - /** @notice If a swap reverts on the destination chain, 1 bytes is sent back instead. This is the byte. */ + /** + * @notice If the message reverts on the destination chain, + * 1 byte is prepended to the original message on ack. This is the byte. + */ bytes1 constant MESSAGE_REVERTED = 0xff; - /** @notice If the original sender is not authorised on the application on the destination chain, 1 bytes is sent back instead. This is the byte. */ + /** + * @notice If the original sender is not authorised on the application on the destination chain, + * 1 byte is prepended to the original message on ack. This is the byte. + */ bytes1 constant NO_AUTHENTICATION = 0xfe; - /** @notice Message timed out on destination chain. */ + /** + * @notice If the message timed out on destination chain, + * 1 byte is prepended to the original message on ack. This is the byte. + */ bytes1 constant MESSAGE_TIMED_OUT = 0xfd; /** - * @notice If setRemoteImplementation is called with this as the - * destination implementation (abi.encodePacked(DISABLE_ROUTE_IMPLEMENTATION)), then the route is - * permanently disabled + * @notice If setRemoteImplementation is called with this as the destination implementation + * (abi.encodePacked(DISABLE_ROUTE_IMPLEMENTATION)), then the route is permanently disabled + * This is treated as a magic value so that incase an implementations treats abi.encodePacked(0x00) + * as a valid destination (say address(0)). */ bytes1 constant DISABLE_ROUTE_IMPLEMENTATION = 0x00; /** - * @notice If a relayer or application provides an address which cannot accept gas and the transfer fails - * the gas is sent here instead. + * @notice If a relayer or application provides an address that cannot accept gas and + * the transfer fails the gas is sent here instead. * @dev This may not invoke any logic on receive() or be a proxy. */ address immutable public SEND_LOST_GAS_TO; //--- Storage ---// + /** * @notice Get incentive description based on message context: messageIdentifier, fromApplication, and destChain - * @dev fromApplication and destChain are required to fetch the bounty since those - * match to exactly 1 remote escrow implementation. As a result, this restricts this storage + * @dev fromApplication and destChain are required to fetch the bounty. Together they + * match exactly 1 remote escrow implementation. As a result, this restricts the storage * slot's security to the security of the specified remote escrow implementation. */ mapping(address fromApplication => mapping(bytes32 destChain => mapping(bytes32 messageIdentifier => IncentiveDescription))) _bounty; @@ -73,16 +88,16 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes mapping(address => mapping(bytes32 => bytes32)) public implementationAddressHash; //--- Virtual Functions ---// - // To integrate a messaging protocol, a contract has to inherit this contract and implement the below 4 functions. + // To integrate a messaging protocol, a contract has to inherit this contract and implement 4 functions below. /** - * @notice Verify a message's authenticity. + * @notice Verifies the authenticity of a message. * @dev Should be overwritten by the specific messaging protocol verification structure. * @param messagingProtocolContext Some context that is useful for verifing the message. - * It should not containt the message but instead verification context like signatures, header, etc. + * It should not contain the message but instead verification context like signatures, header, etc. * Context may not be needed for verifying the message and can be prepended to rawMessage. - * @param rawMessage Some kind of package, initially untrusted. May contain the encoded message but - * the message contained within as a slice. It may contain more than just the message like signatures. + * @param rawMessage Some kind of package, initially untrusted. Should contain the message as a slice + * It may contain more than just the message, like signatures, headers, etc. * * @return sourceIdentifier The source chain identifier. A chainID, a channel ID, or similarly. * @return implementationIdentifier An identifier for the address that emitted the message. @@ -99,7 +114,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes ); /** - * @notice Send the message to the messaging protocol. + * @notice Deliver the message to the messaging protocol to generate a proof. * @dev Should be overwritten to send a message using the specific messaging protocol. * The function is allowed to claim native tokens (set costOfsendPacketInNativeToken). * The function is allowed to take ERC20 tokens (transferFrom(msg.sender,...)) @@ -107,7 +122,8 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes * @param destinationIdentifier The destination chain for the message. * @param destinationImplementation The destination escrow contract. * @param message The message. Contains relevant escrow context. - * @param deadline When the message should be delivered before. If the AMB does not nativly support a timeout on their messages this parameter should be ignored. If 0 is provided, parse it as MAX + * @param deadline A timestamp that the message should be delivered before. If the AMB does not nativly + * support a timeout on their messages this parameter should be ignored. If 0 is provided, parse it as MAX. * @return costOfsendPacketInNativeToken An additional cost to emitting messages in NATIVE tokens. */ function _sendPacket( @@ -123,64 +139,50 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes function _uniqueSourceIdentifier() virtual internal view returns(bytes32 sourceIdentifier); /** - * @notice Returns the duration for which a proof is valid for. - * It may vary by destination. + * @notice The duration for which a proof is valid for. It may vary by destination. * @dev On checks, block.timestamp is added to the return of this function such that * block.timestamp + _proofValidPeriod > deadline. - * Can be set to 0 which implies any is valid. + * If any deadline is valid, set to **0** instead of ~~type(uint64).max~~. * @param destinationIdentifier The destination chain identifier. - * @return duration The duration of when the application's message won't get delivered but rather acked back. + * @return duration The maximum proof duration. Includes the time from message emitted (proof gen) + * to message finalised. */ function _proofValidPeriod(bytes32 destinationIdentifier) virtual internal view returns(uint64 duration); /** - * @notice Returns the duration for which a proof is valid for. + * @notice The duration for which a proof is valid for. * @dev On checks, block.timestamp is added to the return of this function such that * block.timestamp + _proofValidPeriod > deadline. * If 0, implies that any deadline is valid. * @param destinationIdentifier The destination chain identifier. - * @return duration The timestamp of when the application's message won't get delivered but rather acked back. + * @return duration The maximum proof duration. Includes the time from message emitted (proof gen) + * to message finalised. */ function proofValidPeriod(bytes32 destinationIdentifier) external view returns(uint64 duration) { return duration = _proofValidPeriod(destinationIdentifier); } /** - * @param sendLostGasTo It should only be set to an EOA or a contract that has no logic on receive. It cannot be a proxy. - * It cannot be set to address 0, instead use a burn address (0xdead) if no-one wants to take responsibility of the Ether. + * @param sendLostGasTo It should only be set to an EOA or a contract that has no logic on receive nor be a proxy. + * If no-one wants to take responsibility of the lost Ether, use a burn address (0xdead) instead of address(0). */ constructor(address sendLostGasTo) { if (sendLostGasTo == address(0)) revert SendLostGasToIsZero(); SEND_LOST_GAS_TO = sendLostGasTo; } - /** - * @notice Generates a unique message identifier for a message - */ - function _getMessageIdentifier( - uint64 deadline, - bytes32 destinationIdentifier, - bytes calldata message - ) view internal virtual returns(bytes32) { - return _getMessageIdentifier( - msg.sender, - deadline, - block.number, - _uniqueSourceIdentifier(), - destinationIdentifier, - message - ); - } - /** * @notice Generates a unique message identifier for a message * @dev The identifier should: - * - Be unique based on the sender such that applications can't be DoS'ed. - * - Be unique over time: Use blocknumber or blockhash - * - Be unique on the source chain: Use a unique destinationIdentifier - * - Be unique on destination chain: Use a unique source identifier - * - Depend on the message - * This also implies that application should make their messages user specific, say include a user address. + * - Be unique within a trusted ecosystem. + * - Be unique based on the sender such that applications can't be DoS'ed. + * - Contain the deadline for timeout validation. + * - Be unique over time: Use blocknumber or blockhash. + * - Be unique on the source chain: Use a unique destinationIdentifier. + * - Be unique on destination chain: Use a unique source identifier. + * - Depend on the message, for uniqueness & for timeouts. + * Point 2 implies that applications are allowed to DoS themselves. To protect against this, applications + * should include some user unique information, say include a user address. */ function _getMessageIdentifier( address messageSender, @@ -203,6 +205,26 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes ); } + /** + * @notice Generates a unique message identifier for a message + * @dev Simplifies using _getMessageIdentifier raw by assuming what the inputs + * to the other parameters should be. + */ + function _getMessageIdentifier( + uint64 deadline, + bytes32 destinationIdentifier, + bytes calldata message + ) view internal virtual returns(bytes32) { + return _getMessageIdentifier( + msg.sender, + deadline, + block.number, + _uniqueSourceIdentifier(), + destinationIdentifier, + message + ); + } + //--- Getter Functions ---// /** * @notice Returns the bounty associated with a specific messageIdentifier. @@ -216,12 +238,13 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes } /** - * @notice Returns a hash of the ack unless there was no ack then it returns bytes32(uint256(1)); - * If the message hasn't been delivered yet it still returns bytes32(0) + * @notice Get the message statue through the an ack hash. + * If the message hasn't been delivered yet it returns bytes32(0) * @param sourceIdentifier The source chain the message was emitted from. * @param sourceImplementationIdentifier The source escrow implementation that emitted the message. * @param messageIdentifier The message identifier of the message. - * @return hasMessageBeenExecuted Boolean indicating if a message has been executed. + * @return hasMessageBeenExecuted Hash of the ack message. If not ack, then bytes32(uint256(1)). If not + * executed then bytes32(0). */ function messageDelivered( bytes32 sourceIdentifier, @@ -233,11 +256,12 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice Sets the escrow implementation for a specific chain - * @dev This can only be set once. When set, is cannot be changed. + * @dev This can only be set once. When set, it cannot be changed. * This is to protect relayers as this could be used to fail acks. * @param destinationIdentifier An identifier for the destination chain. Varies from AMB. * @param implementation Implementation address. Encoding varies between AMBs. - * You are not allowed to set a 0 length implementation address. + * You are not allowed to set a 0 length implementation address. Beaware that while some implementations + * are valid for sending (say hex"0x01" which may be read as address(uint160(1))), they break acks / delivery. * If you want to disable a specific route, set implementation to hex"00" (DISABLE_ROUTE_IMPLEMENTATION). */ function setRemoteImplementation(bytes32 destinationIdentifier, bytes calldata implementation) external { @@ -300,22 +324,22 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice Set a bounty on a message and transfer the message to the messaging protocol. - * @dev Called by other contracts. - * Any integrating application should check: - * 1. That incentive.maxGasAck is sufficient! Otherwise, an off-chain agent needs to re-submit the ack. - * 2. That incentive.maxGasDelivery is sufficient. Otherwise, the call will fail within the try - catch. + * @dev Called by applications. These application should ensure: + * 1. incentive.maxGasAck is sufficient! Otherwise, an off-chain agent needs to re-submit the ack. + * 2. incentive.maxGasDelivery is sufficient. Otherwise, the call will fail within the try - catch. * 3. The relay incentive is enough to get the message relayed within the expected time. If that is never, this check is not needed. * Furthermore, if the package times out there is no gas refund. - * @param destinationIdentifier 32 bytes which identifies the destination chain. - * @param destinationAddress The destination address encoded in 65 bytes: First byte is the length and last 64 is the destination address. + * Sending too much value results in the excess being refunded to refundGasTo via a call. This may allow refundGasTo to throw the call. + * @param destinationIdentifier 32 bytes that identifies the destination chain. + * @param destinationAddress The destination application encoded in 65 bytes: First byte is the length and last 64 is the destination application. * @param message The message to be sent to the destination. Please ensure the message is block-unique. - * This means that you don't send the same message twice in a single block. + * This means that you don't send the same message twice in a single block. If you need to do that, add a nonce or noice. * @param incentive The incentive to attatch to the bounty. The price of this incentive has to be paid, * any excess is refunded to refundGasTo. (not msg.sender) - * @param deadline After this date, do not allow relayers to execute the message on the destination chain. If set to 0, "disable". - * Not all AMBs may support disabling the deadline. If messages should also be able to be acked back, we recommend setting the deadline far in the future. - * Note that it may still take a significant amount of time to bring back the timeout. - * @return gasRefund The amount of excess gas which was paid to this call. The app should handle the excess. + * @param deadline After this date, do not allow relayers to execute the message on the destination chain. If set to 0, disable timeouts. + * Not all AMBs may support disabling the deadline. If acks are required it is recommended to set the deadline sometime in the future. + * Note, it may still take a significant amount of time to bring back the timeout. + * @return gasRefund The amount of excess gas that was paid to this call. The app should handle the excess. * @return messageIdentifier An unique identifier for a message. */ function submitMessage( @@ -339,7 +363,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes uint64 ambMaxDeadline = _proofValidPeriod(destinationIdentifier); if (ambMaxDeadline != 0 && deadline == 0) revert DeadlineTooLong(ambMaxDeadline, 0); if (ambMaxDeadline != 0 && deadline > uint64(block.timestamp) + ambMaxDeadline) revert DeadlineTooLong(uint64(block.timestamp) + ambMaxDeadline, deadline); - // Check that the deadline is in the future = not (block.timestamp < deadline) = block.timestamp >= deadline + // Check that the deadline is in the future = not (block.timestamp < deadline) = block.timestamp >= deadline. if (deadline != 0 && block.timestamp >= deadline) revert DeadlineInPast(uint64(block.timestamp), deadline); } @@ -349,17 +373,17 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes destinationIdentifier, message ); - // Store the bounty, get the sum for later refunding excess. + // Store the bounty, get the sum to refunding excess later. uint128 sum = _setBounty(msg.sender, destinationIdentifier, messageIdentifier, incentive); // Add escrow context to the message. bytes memory messageWithContext = bytes.concat( - bytes1(CTX_SOURCE_TO_DESTINATION), // This is a sendPacket, - bytes32(messageIdentifier), // A unique message identifier - convertEVMTo65(msg.sender), // Original sender - destinationAddress, // The address to deliver the (original) message to. + bytes1(CTX_SOURCE_TO_DESTINATION), // This is a sendPacket intended for the _destination_ + bytes32(messageIdentifier), // A unique message identifier. + convertEVMTo65(msg.sender), // Original sender / application. + destinationAddress, // The address to deliver the provided message to. bytes8(uint64(deadline)), - bytes6(incentive.maxGasDelivery), // Send the gas limit to the other chain so we can enforce it + bytes6(incentive.maxGasDelivery), // The delivery gas limit, to enforce on the destination. message // The message to deliver to the destination. ); @@ -392,7 +416,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Return excess incentives to the user (found from incentive.refundGasTo). unchecked { if (msg.value > sum) { - // We know: msg.value >= sum, thus msg.value - sum >= 0. + // We know: msg.value > sum, thus msg.value - sum > 0. gasRefund = msg.value - sum; Address.sendValue(payable(incentive.refundGasTo), uint256(gasRefund)); return (gasRefund, messageIdentifier); @@ -402,15 +426,16 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes } /** - * @notice Deliver a message which has been *signed* by a messaging protocol. + * @notice Deliver a message that has been proved by a messaging protocol. * @dev This function is intended to be called by off-chain agents. - * Please ensure that feeRecipient can receive gas token: Either it is an EOA or a implement fallback() / receive(). + * Please ensure that feeRecipient can receive gas token: Either it is an EOA or a implement fallback() / receive() with no logic. * Likewise for any non-evm chains. Otherwise the message fails (ack) or the relay payment is lost (call). * You need to pass in incentive.maxGas(Delivery|Ack) + messaging protocol dependent buffer, otherwise this call might fail. - * On Receive implementations make _verifyPacket revert. The result is - * that this endpoint is disabled. + * + * OnReceive implementations make _verifyPacket revert. The result is that this endpoint is disabled. + * Though the endpoint is _virtual_ so feel free to override. * @param messagingProtocolContext Additional context required to verify the message by the messaging protocol. - * @param rawMessage The raw message as it was emitted. + * @param rawMessage A messaging protocol message, including the message as it was sent as a slice. * @param feeRecipient An identifier for the the fee recipient. The identifier should identify the relayer on the source chain. * For EVM (and this contract as a source), use the bytes32 encoded address. For other VMs you might have to register your address. */ From 7a830ec76013e5df1d4183c3fabe2433f86f1ea1 Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 15 May 2024 11:10:21 +0200 Subject: [PATCH 5/6] feat: further improve documentation --- src/IncentivizedMessageEscrow.sol | 150 +++++++++++------- .../wormhole/IncentivizedWormholeEscrow.sol | 35 +++- src/apps/wormhole/external/callworm/README.md | 2 +- .../external/callworm/WormholeVerifier.sol | 4 + 4 files changed, 129 insertions(+), 62 deletions(-) diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol index 362ab83..d893239 100644 --- a/src/IncentivizedMessageEscrow.sol +++ b/src/IncentivizedMessageEscrow.sol @@ -93,6 +93,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice Verifies the authenticity of a message. * @dev Should be overwritten by the specific messaging protocol verification structure. + * onRecv. implementations should collect acks so _verifyPacket returns true after acks have been executed once. * @param messagingProtocolContext Some context that is useful for verifing the message. * It should not contain the message but instead verification context like signatures, header, etc. * Context may not be needed for verifying the message and can be prepended to rawMessage. @@ -426,14 +427,14 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes } /** - * @notice Deliver a message that has been proved by a messaging protocol. + * @notice Deliver a message & proof of validity. * @dev This function is intended to be called by off-chain agents. - * Please ensure that feeRecipient can receive gas token: Either it is an EOA or a implement fallback() / receive() with no logic. - * Likewise for any non-evm chains. Otherwise the message fails (ack) or the relay payment is lost (call). + * Please ensure that feeRecipient can receive gas token: EOA or implements fallback() / receive() with no logic. + * Likewise for any non-evm chains. Otherwise the message fails (ack) or the relay payment is lost (deliver). * You need to pass in incentive.maxGas(Delivery|Ack) + messaging protocol dependent buffer, otherwise this call might fail. * - * OnReceive implementations make _verifyPacket revert. The result is that this endpoint is disabled. - * Though the endpoint is _virtual_ so feel free to override. + * OnReceive implementations should make _verifyPacket revert. The result is that this function is disabled. + * Though this function is _virtual_ so feel free to override. * @param messagingProtocolContext Additional context required to verify the message by the messaging protocol. * @param rawMessage A messaging protocol message, including the message as it was sent as a slice. * @param feeRecipient An identifier for the the fee recipient. The identifier should identify the relayer on the source chain. @@ -450,7 +451,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Verify that the message is authentic and remove potential context that the messaging protocol added to the message. (bytes32 chainIdentifier, bytes memory implementationIdentifier, bytes calldata message) = _verifyPacket(messagingProtocolContext, rawMessage); - // Figure out if this is a call or an ack. + // Figure out if this is deliver or ack. uint128 cost = 0; bytes1 context = bytes1(message[0]); if (context == CTX_SOURCE_TO_DESTINATION) { @@ -490,22 +491,30 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes //--- Internal Functions ---// /** - * @notice Handles call messages. + * @notice Handles messages deliveries (SOURCE_TO_DESTINATION) */ - function _handleMessage(bytes32 sourceIdentifier, bytes memory sourceImplementationIdentifier, bytes calldata message, bytes32 feeRecipient, uint256 gasLimit) internal returns(bytes memory receiveAckWithContext) { + function _handleMessage( + bytes32 sourceIdentifier, + bytes memory sourceImplementationIdentifier, + bytes calldata message, + bytes32 feeRecipient, + uint256 gasLimit + ) internal returns(bytes memory receiveAckWithContext) { // Ensure message is unique and can only be execyted once bytes32 messageIdentifier = bytes32(message[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]); // The 3 next lines act as a reentry guard, so this call doesn't have to be protected by reentry. // We will re-set _messageDelivered[messageIdentifier] again later as the hash of the ack, however, we need re-entry protection - // so applications don't try to claim incentives multiple times. So, we set it now and change it later. + // so applications don't try to claim incentives multiple times. bytes32 messageState = _messageDelivered[sourceIdentifier][sourceImplementationIdentifier][messageIdentifier]; if (messageState != bytes32(0)) revert MessageAlreadySpent(); + // This is where the "magic-byte" of bytes32(uint256(1)) comes from. Generally, it should always be overwritten by + // an ack hash. _messageDelivered[sourceIdentifier][sourceImplementationIdentifier][messageIdentifier] = bytes32(uint256(1)); // Prepare to deliver the message to application. // We need toApplication to check if the source implementation is valid - // and we optimistically decode fromApplication since it is needed in all cases. + // and we opportunistic decode fromApplication since it is needed in all cases. address toApplication = address(bytes20(message[CTX0_TO_APPLICATION_START_EVM:CTX0_TO_APPLICATION_END])); bytes calldata fromApplication = message[FROM_APPLICATION_LENGTH_POS:FROM_APPLICATION_END]; @@ -518,9 +527,9 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes bytes32 expectedSourceImplementationHash = implementationAddressHash[toApplication][sourceIdentifier]; // Check that the application allows the source implementation. // This is not the case when another implementation calls this contract from the source chain. - // Since this could be a mistake, send back an ack with the relevant information. + // This could be a mistake, send back an ack with the relevant information. if (expectedSourceImplementationHash != keccak256(sourceImplementationIdentifier)) { - // If they are different, return send a failed message back with `0xfe`. + // If they are different, return send a failed message back with NO_AUTHENTICATION. acknowledgement = bytes.concat( NO_AUTHENTICATION, message[CTX0_MESSAGE_START: ] @@ -537,7 +546,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes acknowledgement ); - // Store a hash of the acknowledgement so we can later retry a potentially invalid ack proof. + // Store a hash of the acknowledgement so we can later retry the ack if the ack proofs expires / becomes invalid. _messageDelivered[sourceIdentifier][sourceImplementationIdentifier][messageIdentifier] = keccak256(receiveAckWithContext); // Message has been delivered and shouldn't be executed again. @@ -546,7 +555,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes } // Check that if the deadline has been set (deadline != 0). If the deadline has been set, - // check if the current timestamp is beyond the deadline and return TIMED_OUT if it is. + // check if the current timestamp is beyond the deadline and return MESSAGE_TIMED_OUT if it is. uint64 deadline = uint64(bytes8(message[CTX0_DEADLINE_START:CTX0_DEADLINE_END])); if (deadline != 0 && deadline < block.timestamp) { acknowledgement = bytes.concat( @@ -561,7 +570,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes messageIdentifier, // message identifier fromApplication, feeRecipient, - bytes6(uint48(2**47)), // We set the gas spent as max. Note that 2**47 is likely to be larger than maxGas but it is fine since we are doing max on source. + bytes6(uint48(2**47)), // We set the gas spent as max. Why 2**47 instead of maxGasDelivery? 2**47 is only 1 high bit and it saves gas. Furthermore, min(2**47, maxGasDelivery) is checked on the source. bytes8(uint64(block.timestamp)), // If this overflows, it is fine. It is used in conjunction with a delta. acknowledgement ); @@ -579,7 +588,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Execute call to application. Gas limit is set explicitly to ensure enough gas has been sent. - // This call might fail because the abi.decode of the return value can fail. It is too gas costly to check all correctness + // This call might fail because the abi.decode of the return value can fail. It is too gas costly to check co full correctness // of the returned value and then error if decoding is not possible. // As a result, relayers needs to simulate the tx. If the call fails, then they should blacklist the message. // The call will only fall if the application doesn't expose receiveMessage or captures the message via a fallback. @@ -594,8 +603,8 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes if(gasleft() < maxGas * 1 / 63) revert NotEnoughGasExecution(); // Send the message back if the execution failed. - // This lets you store information in the message that you can trust - // gets returned. (You just have to understand that the status is appended as the first byte.) + // This lets you store information in the message that you can trust gets returned. + // (You just have to understand that the status is appended as the first byte.) acknowledgement = bytes.concat( MESSAGE_REVERTED, message[CTX0_MESSAGE_START: ] @@ -620,17 +629,22 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Because it lets us pop messageIdentifier from the stack. This avoid a stack limit reached error. // Not optimal but okay-ish. - // Send message to messaging protocol - // This is done on processPacket. - // This is done by returning receiveAckWithContext while source identifier and sourceImplementationIdentifier are known. + // Emit event to inform relayers that the message has been delivered. emit MessageDelivered(sourceImplementationIdentifier, sourceIdentifier, messageIdentifier); + // Send message to messaging protocol in processMessage. return receiveAckWithContext; } /** - * @notice Handles ack messages. + * @notice Handles ack messages (DESTINATION_TO_SOURCE). */ - function _handleAck(bytes32 destinationIdentifier, bytes memory destinationImplementationIdentifier, bytes calldata message, bytes32 feeRecipient, uint256 gasLimit) internal { + function _handleAck( + bytes32 destinationIdentifier, + bytes memory destinationImplementationIdentifier, + bytes calldata message, + bytes32 feeRecipient, + uint256 gasLimit + ) internal { // Ensure the bounty can only be claimed once. bytes32 messageIdentifier = bytes32(message[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]); address fromApplication = address(bytes20(message[FROM_APPLICATION_START_EVM:FROM_APPLICATION_END])); @@ -646,7 +660,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes uint96 priceOfAckGas = incentive.priceOfAckGas; uint64 targetDelta = incentive.targetDelta; - // Ensure the bounty can only be claimed once. This call is matched on the ack side, + // Ensure the bounty can only be claimed once. This call is matched on the timeout side, // so it also ensures that an ack cannot be delivered if a timeout has been seen. if (refundGasTo == address(0)) revert MessageAlreadyAcked(); delete _bounty[fromApplication][destinationIdentifier][messageIdentifier]; // The bounty cannot be accessed anymore. @@ -673,8 +687,8 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // fromApplication.call{gas: maxGasAck}( // abi.encodeWithSignature("receiveAck(bytes32,bytes32,bytes)", destinationIdentifier, messageIdentifier, message[CTX1_MESSAGE_START: ]) // ); - } + // External calls are allocated gas according roughly the following: min( gasleft * 63/64, gasArg ). // If there is no check against gasleft, then a relayer could potentially cheat by providing less gas. // Without a check, they only have to provide enough gas such that any further logic executees on 1/64 of gasleft @@ -731,10 +745,17 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes * @dev This function is very light. That is because it is intended to be used for both: * 1. Handling authentic timeouts from some AMBs. * 2. Handling returning timedout functions. - * Check if the destination implementation is correct. This function should never be called checking if the origin - * is trusted. + * Before calling, check if the destination implementation is correct. */ - function _handleTimeout(bytes32 destinationIdentifier, bytes memory destinationImplementationIdentifier, bytes32 messageIdentifier, address fromApplication, bytes calldata applicationMessage, bytes32 feeRecipient, uint256 gasLimit) internal { + function _handleTimeout( + bytes32 destinationIdentifier, + bytes memory destinationImplementationIdentifier, + bytes32 messageIdentifier, + address fromApplication, + bytes calldata applicationMessage, + bytes32 feeRecipient, + uint256 gasLimit + ) internal { // The 3 (9, loading the variables out of storage fills a bit.) next lines act as a reentry guard, // so this call doesn't have to be protected by reentry. IncentiveDescription storage incentive = _bounty[fromApplication][destinationIdentifier][messageIdentifier]; @@ -806,13 +827,22 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes * @notice Verifies the input parameters are contained messageIdentfier and that the other arguments are valid. * The usage of this function is intended when no parameters of a message can be trusted and we have to verify them. * This is the case when we receive a timeout, as the timeout had to be emitted without any verification - * on the remote chain, for us to verify when the know if a message identifier is good AND how to compute it. + * on the remote chain, for us to then verify since we know when a message identifier is good AND how to compute it. * * @dev This function uses the fact that hash(a) == hash(b) IFF a == b. So if someone proposes b, we have hash(a) * then we can check if b == a by hashing b and comparing to a. * a is the initial state when the message was initiated and b is the proposed state from the timeout. + * + * When hash(a) is the message identifier, this allows us to verify authenticity by: + * 1. The package is correctly formatted. The data within matches the message identifier. + * 2. The remote implementation can verify that no package has previously been executed with that message identifier. + * 3. If we check if the message identifier has a bounty, we must have emitted that message. (in _handleTimeout) */ - function _verifyTimeout(bytes32 destinationIdentifier, bytes memory implementationIdentifier, bytes calldata message) internal view returns(bytes32 messageIdentifier, address fromApplication, bytes calldata applicationMessage) { + function _verifyTimeout( + bytes32 destinationIdentifier, + bytes memory implementationIdentifier, + bytes calldata message + ) internal view returns(bytes32 messageIdentifier, address fromApplication, bytes calldata applicationMessage) { // First check if the application trusts the implementation on the destination chain. This is very important // since the remote implementation NEEDS to check that the message hasn't been executed before the deadline // and if the message get relayed post deadline, then it should never arrive at the application. @@ -826,7 +856,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Do we need to check deadline again? // Considering the cost: cheap, we will do it. In most instances it is not needed. // This is because we must expect the remote implementation to also do the check to save gas - // since it is an obvious and valid remote check + // since it is an obvious and valid check on the remote. uint64 deadline = uint64(bytes8(message[CTX2_DEADLINE_START:CTX2_DEADLINE_END])); if (deadline >= block.timestamp || deadline == 0) revert DeadlineNotPassed(deadline, uint64(block.timestamp)); @@ -844,7 +874,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // should have been used to check if the message would have been executed before. // We can check if the message identifier is correct by checking the bounty. (See _handleTimeout) // However, we still need to check the rest of the information. The message identifier has been crafted - // such that it is excatly dependent on the rest of the information we use. + // so it is dependent on the rest of the information we use. applicationMessage = message[CTX2_MESSAGE_START: ]; @@ -858,7 +888,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes applicationMessage ); - // Get the reference message identifier from the package. We need to verify this since we used it on the sending chain. + // Get the reference message identifier from the package. We need to verify this since it was used on the sending chain. messageIdentifier = bytes32(message[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]); if (computedMessageIdentifier != messageIdentifier) revert InvalidTimeoutPackage(messageIdentifier, computedMessageIdentifier); } @@ -902,7 +932,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes refund = maxFee - actualFee; } - // send is used to ensure this doesn't revert. Transfer could revert and block the ack from ever being delivered. + // send is used to ensure this doesn't revert. ".transfer" could revert and block the ack from ever being delivered. if(!payable(refundGasTo).send(refund)) { payable(SEND_LOST_GAS_TO).transfer(refund); // If we don't send the gas somewhere, the gas is lost forever. } @@ -920,7 +950,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // The result is that this contract still has deliveryFee. As a result, send it somewhere else. payable(SEND_LOST_GAS_TO).transfer(deliveryFee); // If we don't send the gas somewhere, the gas is lost forever. } - payable(sourceFeeRecipient).transfer(ackFee); // If this reverts, then the relayer that is executing this tx provided a bad input. + Address.sendValue(payable(sourceFeeRecipient), ackFee); // If this reverts, then the relayer that is executing this tx provided a bad input. return (gasSpentOnSource, deliveryFee, ackFee); } @@ -940,36 +970,37 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // should get everything. if (executionTime > 32768 days) executionTime = 0; } - // The incentive scheme is as follows: When executionTime = targetDelta then - // The rewards are distributed as per the incentive spec. If the time is less, then + // The incentive scheme is as follows: When executionTime = targetDelta then + // the rewards are distributed as per the incentive spec. If the time is less, then // more incentives are given to the destination relayer while if the time is more, // then more incentives are given to the sourceRelayer. uint256 forDestinationRelayer = deliveryFee; unchecked { - // |targetDelta - executionTime| < |2**64 + 2**64| = 2**65 - int256 timeBetweenTargetAndExecution = int256(uint256(executionTime))-int256(uint256(targetDelta)); + // |executionTime - targetDelta| < |2**64 + 2**64| = 2**65 + int256 timeBetweenTargetAndExecution = int256(uint256(executionTime)) - int256(uint256(targetDelta)); if (timeBetweenTargetAndExecution <= 0) { // Less time than target passed and the destination relayer should get a larger chunk. - // targetDelta != 0, we checked for that. - // max abs timeBetweenTargetAndExecution = | - targetDelta| = targetDelta => ackFee * targetDelta < actualFee * targetDelta - // 2**127 * 2**64 = 2**191 + // targetDelta != 0, we checked for that. + // max abs timeBetweenTargetAndExecution = | - targetDelta| = targetDelta + // => ackFee * targetDelta < actualFee * targetDelta < 2**127 * 2**64 = 2**191 forDestinationRelayer += ackFee * uint256(- timeBetweenTargetAndExecution) / targetDelta; - } else { + } else { // timeBetweenTargetAndExecution > 0 // More time than target passed and the ack relayer should get a larger chunk. // If more time than double the target passed, the ack relayer should get everything if (uint256(timeBetweenTargetAndExecution) < targetDelta) { // targetDelta != 0, we checked for that. - // max abs timeBetweenTargetAndExecution = targetDelta since we have the above check - // => deliveryFee * targetDelta < actualFee * targetDelta < 2**127 * 2**64 = 2**191 + // max abs timeBetweenTargetAndExecution = targetDelta from previous + // => deliveryFee * targetDelta < actualFee * targetDelta < 2**127 * 2**64 = 2**191 forDestinationRelayer -= deliveryFee * uint256(timeBetweenTargetAndExecution) / targetDelta; - } else { + } else { + // timeBetweenTargetAndExecution > targetDelta === executionTime - targetDelta > targetDelta === executionTime > 2 * targetDelta // This doesn't discourage relaying, since executionTime first begins counting once the destination call has been executed. // As a result, this only encourages delivery of the ack. forDestinationRelayer = 0; } } } - // send is used to ensure this doesn't revert. Transfer could revert and block the ack from ever being delivered. + // send is used to ensure this doesn't revert. ".transfer" could revert and block the ack from ever being delivered. if(!payable(destinationFeeRecipient).send(forDestinationRelayer)) { payable(SEND_LOST_GAS_TO).transfer(forDestinationRelayer); // If we don't send the gas somewhere, the gas is lost forever. } @@ -979,14 +1010,21 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // min forDestinationRelayer = 0 => actualFee - 0 = actualFee forSourceRelayer = actualFee - forDestinationRelayer; } - payable(sourceFeeRecipient).transfer(forSourceRelayer); // If this reverts, then the relayer that is executing this tx provided a bad input. + Address.sendValue(payable(sourceFeeRecipient), forSourceRelayer); // If this reverts, then the relayer that is executing this tx provided a bad input. return (gasSpentOnSource, forDestinationRelayer, forSourceRelayer); } /** * @notice Sets a bounty for a message - * @dev Doesn't check if enough incentives have been provided. + * @dev Does not check if enough incentives have been provided, this is delegated as responsiblity + * of the caller of this function. + * @param fromApplication The application that called the contract. Should generally be msg.sender. Is used to separate storage between applications. + * @param destinationIdentifier The destination chain. Combined with fromApplication, this specifics a unique remote escrow implementation. + * @param messageIdentifier A unique identifier for the message. Is used to check if bounties have been paid, on ack and timeout. + * @param incentive The incentive structure. ".refundGasTo" is not allowed to be address(0). + * + * @return sum The total cost of the bounty. It not checked against msg.value in this contract. */ function _setBounty( address fromApplication, @@ -1005,10 +1043,11 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes /** * @notice Allows anyone to re-execute an ack which didn't properly execute. - * @dev No applciation should rely on this function. It should only be used in-case an - * application has faulty logic. + * @dev No applciation should rely on this function. It should only be used incase an application has faulty logic. * Example: Faulty logic results in wrong enforcement on gas limit => out of gas? * + * This function allows replaying acks. + * * For further parameter documentation, read processPacket. * @param messagingProtocolContext Argument that once made _verifyPacket pass on processPacket * @param rawMessage Argument that once made _verifyPacket pass on processPacket. @@ -1017,6 +1056,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes bytes calldata messagingProtocolContext, bytes calldata rawMessage ) external { + // onRecv. implementations should collect acks so _verifyPacket returns true after first execution of the ack. (bytes32 chainIdentifier, bytes memory implementationIdentifier, bytes calldata message) = _verifyPacket(messagingProtocolContext, rawMessage); bytes1 context = bytes1(message[0]); @@ -1046,7 +1086,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes * the message can only be sent to the correct source chain. * @param implementationIdentifier Which escrow contract to send the ack to? Is checked against stored ack hash * to ensure the message can only be sent to the original implementation. - * @param receiveAckWithContext The message as this contract initially delivered to _sendMessage. + * @param receiveAckWithContext The message as this contract delivered to _sendMessage via processMessage. */ function reemitAckMessage( bytes32 sourceIdentifier, @@ -1056,7 +1096,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // Has the package previously been executed? (otherwise timeout might be more appropiate) // Load the messageIdentifier from receiveAckWithContext. - // This makes it ever so slighly easier to retry messages. + // This makes it slighly easier to retry messages. bytes32 messageIdentifier = bytes32(receiveAckWithContext[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]); bytes32 storedAckHash = _messageDelivered[sourceIdentifier][implementationIdentifier][messageIdentifier]; @@ -1096,7 +1136,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes * @param implementationIdentifier The address of the source generalisedIncentives that emitted the original message * @param originBlockNumber The block number when the message was originally emitted. * Note that for some L2 this could be the block number of the underlying chain. - * Regardless: It is the same block number which generated themessage identifier. + * Regardless: It is the same block number that originally generated themessage identifier. * @param message Original Generalised Incentives message */ function timeoutMessage( @@ -1124,6 +1164,8 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes // ensuring that the original relayer isn't cheated. // When the message arrives, the usual incentive check ensures only 1 message can arrive. Since the incentive check is based on // messageIdentifier, we need to verify it. + // Remember, the messageIdentifier is actually untrusted. So it is trivial to pass the above check. However, any way to pass + // the above check fradulently would result in messageIdentifier being wrong and unable to be reproduced on the source chain. // Load the deadline from the message. uint64 deadline = uint64(bytes8(message[CTX0_DEADLINE_START:CTX0_DEADLINE_END])); diff --git a/src/apps/wormhole/IncentivizedWormholeEscrow.sol b/src/apps/wormhole/IncentivizedWormholeEscrow.sol index 1977c16..a6620c3 100644 --- a/src/apps/wormhole/IncentivizedWormholeEscrow.sol +++ b/src/apps/wormhole/IncentivizedWormholeEscrow.sol @@ -7,7 +7,20 @@ import { SmallStructs } from "./external/callworm/SmallStructs.sol"; import { WormholeVerifier } from "./external/callworm/WormholeVerifier.sol"; import { IWormhole } from "./interfaces/IWormhole.sol"; -// This is a mock contract which should only be used for testing. +/** + * @title Incentivized Wormhole Message Escrow + * @notice Incentivizes Wormhole messages through Generalised Incentives. + * Wormhole does not have any native way of relaying messages, this implemention adds one. + * + * When using Wormhole with Generalised Incentives and you don't want to lose message, be very careful regarding + * emitting messages to destinationChainIdentifiers that does not exist. Wormhole has no way to verify if a + * chain identifier exists or not. If the chain identifier does not exist, it is not possible to get a timeout or ack back. + * + * @dev This implementation only uses the Wormhole contracts to emit messages, it does not use the Wormhole contracts + * to verify if messages are authentic. A custom verification library is used that is more gas efficient and skips + * parts of the VAA that is not relevant to us. This provides significant gas savings compared to verifying packages + * against the Wormhole implementation. + */ contract IncentivizedWormholeEscrow is IncentivizedMessageEscrow, WormholeVerifier { error BadChainIdentifier(); @@ -32,6 +45,7 @@ contract IncentivizedWormholeEscrow is IncentivizedMessageEscrow, WormholeVerifi amount = WORMHOLE.messageFee(); } + /** @notice Wormhole proofs are valid until the guardian set is changed. The new guradian set may sign a new VAA */ function _proofValidPeriod(bytes32 /* destinationIdentifier */) override internal pure returns(uint64) { return 0; } @@ -40,8 +54,11 @@ contract IncentivizedWormholeEscrow is IncentivizedMessageEscrow, WormholeVerifi return sourceIdentifier = UNIQUE_SOURCE_IDENTIFIER; } + /** @dev _message is the entire Wormhole VAA. It contains both the proof & the message as a slice. */ function _verifyPacket(bytes calldata /* _metadata */, bytes calldata _message) internal view override returns(bytes32 sourceIdentifier, bytes memory implementationIdentifier, bytes calldata message_) { + // Decode & verify the VAA. + // This uses the custom verification logic found in ./external/callworm/WormholeVerifier.sol. ( SmallStructs.SmallVM memory vm, bytes calldata payload, @@ -49,16 +66,17 @@ contract IncentivizedWormholeEscrow is IncentivizedMessageEscrow, WormholeVerifi string memory reason ) = parseAndVerifyVM(_message); + // This is the preferred flow used by Wormhole. require(valid, reason); - - // Local "supposedly" this chain identifier. - bytes32 thisChainIdentifier = bytes32(payload[0:32]); + // We added the destination chain to the payload since Wormhole messages are broadcast. + // Get the chain identifier for the destination chain according to the payload. + bytes32 destinationChainIdentifier = bytes32(payload[0:32]); // Check that the message is intended for this chain. - if (thisChainIdentifier != UNIQUE_SOURCE_IDENTIFIER) revert BadChainIdentifier(); + if (destinationChainIdentifier != UNIQUE_SOURCE_IDENTIFIER) revert BadChainIdentifier(); - // Local the identifier for the source chain. + // Get the identifier for the source chain. sourceIdentifier = bytes32(uint256(vm.emitterChainId)); // Load the identifier for the calling contract. @@ -68,11 +86,14 @@ contract IncentivizedWormholeEscrow is IncentivizedMessageEscrow, WormholeVerifi message_ = payload[32:]; } + /** + * @dev Wormhole messages are broadcast, as a result we set destinationChainIdentifier in the message. + */ function _sendPacket(bytes32 destinationChainIdentifier, bytes memory /* destinationImplementation */, bytes memory message, uint64 /* deadline */) internal override returns(uint128 costOfsendPacketInNativeToken) { // Get the cost of sending wormhole messages. costOfsendPacketInNativeToken = uint128(WORMHOLE.messageFee()); - // Relayers can collect the destination chain from the payload and destination address from storage. + // Relayers can collect the destination chain from the payload and destinationImplementation from storage / their whitelist. // Handoff the message to wormhole. WORMHOLE.publishMessage{value: costOfsendPacketInNativeToken}( diff --git a/src/apps/wormhole/external/callworm/README.md b/src/apps/wormhole/external/callworm/README.md index 9060b8d..d005af1 100644 --- a/src/apps/wormhole/external/callworm/README.md +++ b/src/apps/wormhole/external/callworm/README.md @@ -1 +1 @@ -This is an alternative implementation of the wormhole message verification with the purpose of significantly reducing gas cost but also simplify integration by always keeping the message in calldata. \ No newline at end of file +This is an alternative implementation of the wormhole message verification with the purpose of significantly reducing gas cost but also simplify integration by decoding the message as a calldata slice. \ No newline at end of file diff --git a/src/apps/wormhole/external/callworm/WormholeVerifier.sol b/src/apps/wormhole/external/callworm/WormholeVerifier.sol index 3d181b8..b06d1b4 100644 --- a/src/apps/wormhole/external/callworm/WormholeVerifier.sol +++ b/src/apps/wormhole/external/callworm/WormholeVerifier.sol @@ -8,6 +8,10 @@ import "./SmallStructs.sol"; import "../wormhole/libraries/external/BytesLib.sol"; +/** + * @notice Optimised message verifier for Wormhole + * @dev Is based on the Wormhole verification library. + */ contract WormholeVerifier is GettersGetter { using BytesLib for bytes; From fdb20579da7b7caf28f1f6f6b8492001ce4823be Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 15 May 2024 11:16:01 +0200 Subject: [PATCH 6/6] feat: fix audit reference --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 34a682e..b219efa 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -Contracts within this repo have not been audited and should not be used in production. +This repository has been audited 3 times, twice by Veridise and once by Ackee. You can find the audits in `/audits`. # Generalised Incentive Escrow