From 737c5fe24cc679805f21d5eafefac41654896a12 Mon Sep 17 00:00:00 2001 From: shotaronowhere Date: Tue, 12 Apr 2022 21:21:23 +0100 Subject: [PATCH] governance minimization and bug fixes --- .../bridge/FastBridgeReceiverOnEthereum.sol | 50 +++----------- .../src/bridge/FastBridgeSenderToEthereum.sol | 68 ++++++++----------- .../bridge/SafeBridgeReceiverOnEthereum.sol | 31 ++------- .../bridge/interfaces/IFastBridgeReceiver.sol | 5 -- .../bridge/interfaces/IFastBridgeSender.sol | 6 ++ .../src/gateway/ForeignGatewayOnEthereum.sol | 21 +++++- .../src/gateway/HomeGatewayToEthereum.sol | 11 +++ .../gateway/interfaces/IForeignGateway.sol | 1 + 8 files changed, 78 insertions(+), 115 deletions(-) diff --git a/contracts/src/bridge/FastBridgeReceiverOnEthereum.sol b/contracts/src/bridge/FastBridgeReceiverOnEthereum.sol index e70cd9f07..9021e6216 100644 --- a/contracts/src/bridge/FastBridgeReceiverOnEthereum.sol +++ b/contracts/src/bridge/FastBridgeReceiverOnEthereum.sol @@ -46,36 +46,26 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid // * Storage * // // ************************************* // - uint256 public constant ONE_BASIS_POINT = 1e4; // One basis point, for scaling. uint256 public override claimDeposit; // The deposit required to submit a claim. uint256 public override challengeDeposit; // The deposit required to submit a challenge. uint256 public override challengeDuration; // The duration of the period allowing to challenge a claim. - uint256 public override alpha; // Basis point of claim or challenge deposit that are lost when dishonest. mapping(uint256 => Ticket) public tickets; // The tickets by ticketID. - /** * @dev Constructor. - * @param _governor The governor's address. - * @param _safeBridgeSender The address of the Safe Bridge sender on Arbitrum. * @param _inbox The address of the Arbitrum Inbox contract. * @param _claimDeposit The deposit amount to submit a claim in wei. * @param _challengeDeposit The deposit amount to submit a challenge in wei. * @param _challengeDuration The duration of the period allowing to challenge a claim. - * @param _alpha Basis point of claim or challenge deposit that are lost when dishonest. */ constructor( - address _governor, - address _safeBridgeSender, address _inbox, uint256 _claimDeposit, uint256 _challengeDeposit, - uint256 _challengeDuration, - uint256 _alpha - ) SafeBridgeReceiverOnEthereum(_governor, _safeBridgeSender, _inbox) { + uint256 _challengeDuration + ) SafeBridgeReceiverOnEthereum(_inbox) { claimDeposit = _claimDeposit; challengeDeposit = _challengeDeposit; challengeDuration = _challengeDuration; - alpha = _alpha; } // ************************************* // @@ -127,18 +117,18 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid /** * @dev Relay the message for this `ticketID` if the challenge period has passed and the claim is unchallenged. The hash computed over `messageData` and the other parameters must match the hash provided by the claim. * @param _ticketID The ticket identifier referring to a message going through the bridge. - * @param _blockNumber The block number on the cross-domain chain when the message with this ticketID has been sent. + * @param _blocknumber The block number on the cross-domain chain when the message with this ticketID has been sent. * @param _messageData The data on the cross-domain chain for the message sent with this ticketID. */ function verifyAndRelay( uint256 _ticketID, - uint256 _blockNumber, + uint256 _blocknumber, bytes calldata _messageData ) external override { Ticket storage ticket = tickets[_ticketID]; require(ticket.claim.bridger != address(0), "Claim does not exist"); require( - ticket.claim.messageHash == keccak256(abi.encode(_ticketID, _blockNumber, _messageData)), + ticket.claim.messageHash == keccak256(abi.encode(_ticketID, _blocknumber, _messageData)), "Invalid hash" ); require(ticket.claim.claimedAt + challengeDuration < block.timestamp, "Challenge period not over"); @@ -154,12 +144,12 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid * Note: Access restricted to the Safe Bridge. * @dev Relay the message for this `ticketID` as provided by the Safe Bridge. Resolve a challenged claim for this `ticketID` if any. * @param _ticketID The ticket identifier referring to a message going through the bridge. - * @param _blockNumber The block number on the cross-domain chain when the message with this ticketID has been sent. + * @param _blocknumber The block number on the cross-domain chain when the message with this ticketID has been sent. * @param _messageData The data on the cross-domain chain for the message sent with this ticketID. */ function verifyAndRelaySafe( uint256 _ticketID, - uint256 _blockNumber, + uint256 _blocknumber, bytes calldata _messageData ) external override { require(isSentBySafeBridge(), "Access not allowed: SafeBridgeSender only."); @@ -168,7 +158,7 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid require(ticket.relayed == false, "Message already relayed"); // Claim assessment if any - bytes32 messageHash = keccak256(abi.encode(_ticketID, _blockNumber, _messageData)); + bytes32 messageHash = keccak256(abi.encode(_ticketID, _blocknumber, _messageData)); if (ticket.claim.bridger != address(0) && ticket.claim.messageHash == messageHash) { ticket.claim.verified = true; } @@ -187,7 +177,7 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid require(ticket.claim.bridger != address(0), "Claim does not exist"); require(ticket.claim.verified == true, "Claim not verified: deposit forfeited"); - uint256 amount = ticket.claim.claimDeposit + (ticket.challenge.challengeDeposit * alpha) / ONE_BASIS_POINT; + uint256 amount = ticket.claim.claimDeposit + ticket.challenge.challengeDeposit / 2; ticket.claim.claimDeposit = 0; ticket.challenge.challengeDeposit = 0; payable(ticket.claim.bridger).send(amount); // Use of send to prevent reverting fallback. User is responsibility for accepting ETH. @@ -204,7 +194,7 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid require(ticket.challenge.challenger != address(0), "Challenge does not exist"); require(ticket.claim.verified == false, "Claim verified: deposit forfeited"); - uint256 amount = ticket.challenge.challengeDeposit + (ticket.claim.claimDeposit * alpha) / ONE_BASIS_POINT; + uint256 amount = ticket.challenge.challengeDeposit + ticket.claim.claimDeposit / 2; ticket.claim.claimDeposit = 0; ticket.challenge.challengeDeposit = 0; payable(ticket.challenge.challenger).send(amount); // Use of send to prevent reverting fallback. User is responsibility for accepting ETH. @@ -229,26 +219,6 @@ contract FastBridgeReceiverOnEthereum is SafeBridgeReceiverOnEthereum, IFastBrid return (start, end); } - // ************************ // - // * Governance * // - // ************************ // - - function changeClaimDeposit(uint256 _claimDeposit) external onlyByGovernor { - claimDeposit = _claimDeposit; - } - - function changeChallengeDeposit(uint256 _challengeDeposit) external onlyByGovernor { - challengeDeposit = _challengeDeposit; - } - - function changeChallengePeriodDuration(uint256 _challengeDuration) external onlyByGovernor { - challengeDuration = _challengeDuration; - } - - function changeAlpha(uint256 _alpha) external onlyByGovernor { - alpha = _alpha; - } - // ************************ // // * Internal * // // ************************ // diff --git a/contracts/src/bridge/FastBridgeSenderToEthereum.sol b/contracts/src/bridge/FastBridgeSenderToEthereum.sol index 20cee36c7..c120e177c 100644 --- a/contracts/src/bridge/FastBridgeSenderToEthereum.sol +++ b/contracts/src/bridge/FastBridgeSenderToEthereum.sol @@ -25,7 +25,7 @@ contract FastBridgeSenderToEthereum is SafeBridgeSenderToEthereum, IFastBridgeSe struct Ticket { bytes32 messageHash; - uint256 blockNumber; + uint256 blocknumber; bool sentSafe; } @@ -33,35 +33,18 @@ contract FastBridgeSenderToEthereum is SafeBridgeSenderToEthereum, IFastBridgeSe // * Storage * // // ************************************* // - address public governor; // The governor of the contract. IFastBridgeReceiver public fastBridgeReceiver; // The address of the Fast Bridge on Ethereum. - address public fastBridgeSender; // The address of the Fast Bridge sender on Arbitrum, generally the Home Gateway. uint256 public currentTicketID = 1; // Zero means not set, start at 1. mapping(uint256 => Ticket) public tickets; // The tickets by ticketID. - // ************************************* // - // * Function Modifiers * // - // ************************************* // - - modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); - _; - } - /** * @dev Constructor. - * @param _governor The governor's address. * @param _fastBridgeReceiver The address of the Fast Bridge on Ethereum. - * @param _fastBridgeSender The address of the Fast Bridge sender on Arbitrum, generally the Home Gateway. */ constructor( - address _governor, - IFastBridgeReceiver _fastBridgeReceiver, - address _fastBridgeSender + IFastBridgeReceiver _fastBridgeReceiver ) SafeBridgeSenderToEthereum() { - governor = _governor; fastBridgeReceiver = _fastBridgeReceiver; - fastBridgeSender = _fastBridgeSender; } // ************************************* // @@ -69,40 +52,48 @@ contract FastBridgeSenderToEthereum is SafeBridgeSenderToEthereum, IFastBridgeSe // ************************************* // /** - * Note: Access restricted to the `fastSender`, generally the Gateway. * @dev Sends an arbitrary message to Ethereum using the Fast Bridge. * @param _receiver The address of the contract on Ethereum which receives the calldata. * @param _calldata The receiving domain encoded message data. * @return ticketID The identifier to provide to sendSafeFallback(). */ function sendFast(address _receiver, bytes memory _calldata) external override returns (uint256 ticketID) { - require(msg.sender == fastBridgeSender, "Access not allowed: Fast Sender only."); - + require(_calldata.length >= 4, "Malformed calldata: lacks function selector."); ticketID = currentTicketID++; - (bytes32 messageHash, bytes memory messageData) = _encode(ticketID, _receiver, _calldata); - emit OutgoingMessage(ticketID, block.number, _receiver, messageHash, messageData); + (bytes32 messageHash, ) = _encode( + ticketID, + block.number, + msg.sender, + _receiver, + _calldata); + emit OutgoingMessage(ticketID, block.number, msg.sender, _receiver, messageHash, _calldata); - tickets[ticketID] = Ticket({messageHash: messageHash, blockNumber: block.number, sentSafe: false}); + tickets[ticketID] = Ticket({messageHash: messageHash, blocknumber: block.number, sentSafe: false}); } + /** * @dev Sends an arbitrary message to Ethereum using the Safe Bridge, which relies on Arbitrum's canonical bridge. It is unnecessary during normal operations but essential only in case of challenge. * @param _ticketID The ticketID as returned by `sendFast()`. + * @param _fastMsgSender The msg.sender which called sendFast() to register this ticketID. * @param _receiver The address of the contract on Ethereum which receives the calldata. * @param _calldata The receiving domain encoded message data. */ function sendSafeFallback( uint256 _ticketID, + address _fastBridgeReceiver, + address _fastMsgSender, address _receiver, bytes memory _calldata ) external payable override { // TODO: check if keeping _calldata in storage in sendFast() is cheaper than passing it again as a parameter here + // However calldata gas cost-benefit analysis will change with EIP-4488. Ticket storage ticket = tickets[_ticketID]; require(ticket.messageHash != 0, "Ticket does not exist."); require(ticket.sentSafe == false, "Ticket already sent safely."); - (bytes32 messageHash, bytes memory messageData) = _encode(_ticketID, _receiver, _calldata); + (bytes32 messageHash, bytes memory messageData) = _encode(_ticketID, ticket.blocknumber, _fastMsgSender, _receiver, _calldata); require(ticket.messageHash == messageHash, "Invalid message for ticketID."); // Safe Bridge message envelope @@ -110,20 +101,12 @@ contract FastBridgeSenderToEthereum is SafeBridgeSenderToEthereum, IFastBridgeSe bytes memory safeMessageData = abi.encodeWithSelector( methodSelector, _ticketID, - ticket.blockNumber, + ticket.blocknumber, messageData ); - + ticket.sentSafe = true; // TODO: how much ETH should be provided for bridging? add an ISafeBridgeSender.bridgingCost() if needed - _sendSafe(address(fastBridgeReceiver), safeMessageData); - } - - // ************************ // - // * Governance * // - // ************************ // - - function changeFastSender(address _fastBridgeSender) external onlyByGovernor { - fastBridgeSender = _fastBridgeSender; + _sendSafe(address(_fastBridgeReceiver), safeMessageData); } // ************************ // @@ -132,13 +115,16 @@ contract FastBridgeSenderToEthereum is SafeBridgeSenderToEthereum, IFastBridgeSe function _encode( uint256 _ticketID, + uint256 _blocknumber, + address _msgSender, address _receiver, bytes memory _calldata - ) internal view returns (bytes32 messageHash, bytes memory messageData) { - // Encode the receiver address with the function signature + arguments i.e calldata - messageData = abi.encode(_receiver, _calldata); + ) internal pure returns (bytes32 messageHash, bytes memory messageData) { + // Encode the receiver address with the function signature + _msgSender as the first argument, then the rest of the args + (bytes4 functionSelector, bytes memory _args) = abi.decode(_calldata, (bytes4, bytes)); + messageData = abi.encode(_receiver, abi.encodeWithSelector(functionSelector, _msgSender, _args)); // Compute the hash over the message header (ticketID, blockNumber) and body (data). - messageHash = keccak256(abi.encode(_ticketID, block.number, messageData)); + messageHash = keccak256(abi.encode(_ticketID, _blocknumber, messageData)); } } diff --git a/contracts/src/bridge/SafeBridgeReceiverOnEthereum.sol b/contracts/src/bridge/SafeBridgeReceiverOnEthereum.sol index 07a43f833..3241ece5d 100644 --- a/contracts/src/bridge/SafeBridgeReceiverOnEthereum.sol +++ b/contracts/src/bridge/SafeBridgeReceiverOnEthereum.sol @@ -23,35 +23,24 @@ contract SafeBridgeReceiverOnEthereum is ISafeBridgeReceiver { // * Storage * // // ************************************* // - address public governor; // The governor of the contract. address public safeBridgeSender; // The address of the Safe Bridge sender on Arbitrum. IInbox public inbox; // The address of the Arbitrum Inbox contract. - // ************************************* // - // * Function Modifiers * // - // ************************************* // - - modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); - _; - } - /** * @dev Constructor. - * @param _governor The governor's address. * @param _safeBridgeSender The address of the Safe Bridge sender on Arbitrum. * @param _inbox The address of the Arbitrum Inbox contract. */ constructor( - address _governor, - address _safeBridgeSender, address _inbox ) { - governor = _governor; inbox = IInbox(_inbox); - safeBridgeSender = _safeBridgeSender; } + function setSafeBridgeSender(address _safeBridgeSender) external { + if (safeBridgeSender == address(0) ) + safeBridgeSender = _safeBridgeSender; + } // ************************************* // // * Views * // // ************************************* // @@ -60,16 +49,4 @@ contract SafeBridgeReceiverOnEthereum is ISafeBridgeReceiver { IOutbox outbox = IOutbox(inbox.bridge().activeOutbox()); return outbox.l2ToL1Sender() == safeBridgeSender; } - - // ************************ // - // * Governance * // - // ************************ // - - function setSafeBridgeSender(address _safeBridgeSender) external onlyByGovernor { - safeBridgeSender = _safeBridgeSender; - } - - function setInbox(address _inbox) external onlyByGovernor { - inbox = IInbox(_inbox); - } } diff --git a/contracts/src/bridge/interfaces/IFastBridgeReceiver.sol b/contracts/src/bridge/interfaces/IFastBridgeReceiver.sol index 0b508f8d1..871cf094d 100644 --- a/contracts/src/bridge/interfaces/IFastBridgeReceiver.sol +++ b/contracts/src/bridge/interfaces/IFastBridgeReceiver.sol @@ -109,9 +109,4 @@ interface IFastBridgeReceiver { * @return amount The duration of the period allowing to challenge a claim. */ function challengeDuration() external view returns (uint256 amount); - - /** - * @return amount Basis point of claim or challenge deposit that are lost when dishonest. - */ - function alpha() external view returns (uint256 amount); } diff --git a/contracts/src/bridge/interfaces/IFastBridgeSender.sol b/contracts/src/bridge/interfaces/IFastBridgeSender.sol index c5ed12ab8..080704923 100644 --- a/contracts/src/bridge/interfaces/IFastBridgeSender.sol +++ b/contracts/src/bridge/interfaces/IFastBridgeSender.sol @@ -11,6 +11,7 @@ interface IFastBridgeSender { * @dev The Fast Bridge participants need to watch for these events and relay the messageHash on the FastBridgeReceiverOnEthereum. * @param ticketID The ticket identifier referring to a message going through the bridge. * @param blockNumber The block number when the message with this ticketID has been created. + * @param messageSender The address of the cross-domain receiver of the message, generally the Foreign Gateway. * @param target The address of the cross-domain receiver of the message, generally the Foreign Gateway. * @param messageHash The hash uniquely identifying this message. * @param message The message data. @@ -18,6 +19,7 @@ interface IFastBridgeSender { event OutgoingMessage( uint256 indexed ticketID, uint256 blockNumber, + address messageSender, address target, bytes32 indexed messageHash, bytes message @@ -39,11 +41,15 @@ interface IFastBridgeSender { /** * @dev Sends an arbitrary message across domain using the Safe Bridge, which relies on the chain's canonical bridge. It is unnecessary during normal operations but essential only in case of challenge. * @param _ticketID The ticketID as returned by `sendFast()`. + * @param _fastBridgeReceiver The address of the fast bridge receiver deployment. + * @param _fastMsgSender The msg.sender which called sendFast() * @param _receiver The cross-domain contract address which receives the calldata. * @param _calldata The receiving domain encoded message data. */ function sendSafeFallback( uint256 _ticketID, + address _fastBridgeReceiver, + address _fastMsgSender, address _receiver, bytes memory _calldata ) external payable; diff --git a/contracts/src/gateway/ForeignGatewayOnEthereum.sol b/contracts/src/gateway/ForeignGatewayOnEthereum.sol index c0614bc72..c7cb74a0d 100644 --- a/contracts/src/gateway/ForeignGatewayOnEthereum.sol +++ b/contracts/src/gateway/ForeignGatewayOnEthereum.sol @@ -45,6 +45,8 @@ contract ForeignGatewayOnEthereum is IForeignGateway { address public governor; IFastBridgeReceiver public fastbridge; + IFastBridgeReceiver public depreciatedFastbridge; + uint256 fastbridgeExpiration; address public homeGateway; event OutgoingDispute( @@ -57,7 +59,10 @@ contract ForeignGatewayOnEthereum is IForeignGateway { ); modifier onlyFromFastBridge() { - require(address(fastbridge) == msg.sender, "Access not allowed: Fast Bridge only."); + require( + address(fastbridge) == msg.sender || + ( (block.timestamp < fastbridgeExpiration) && address(depreciatedFastbridge) == msg.sender) + , "Access not allowed: Fast Bridge only."); _; } @@ -84,6 +89,16 @@ contract ForeignGatewayOnEthereum is IForeignGateway { } } + /** @dev Changes the fastBridge, useful to increase the claim deposit. + * @param _fastbridge The address of the new fastBridge. + */ + function changeFastbridge(IFastBridgeReceiver _fastbridge) external onlyByGovernor { + // grace period to relay remaining messages in the relay / bridging process + fastbridgeExpiration = block.timestamp + _fastbridge.challengeDuration() + 1209600; // 2 weeks + depreciatedFastbridge = fastbridge; + fastbridge = _fastbridge; + } + /** @dev Changes the `feeForJuror` property value of a specified subcourt. * @param _subcourtID The ID of the subcourt. * @param _feeForJuror The new value for the `feeForJuror` property value. @@ -130,10 +145,12 @@ contract ForeignGatewayOnEthereum is IForeignGateway { * Relay the rule call from the home gateway to the arbitrable. */ function relayRule( + address _messageOrigin, bytes32 _disputeHash, uint256 _ruling, address _relayer - ) external onlyFromFastBridge { + ) external onlyFromFastBridge(){ + require(_messageOrigin == homeGateway, "Only the homegateway is allowed."); DisputeData storage dispute = disputeHashtoDisputeData[_disputeHash]; require(dispute.id != 0, "Dispute does not exist"); diff --git a/contracts/src/gateway/HomeGatewayToEthereum.sol b/contracts/src/gateway/HomeGatewayToEthereum.sol index 6ce6cff60..d22124e26 100644 --- a/contracts/src/gateway/HomeGatewayToEthereum.sol +++ b/contracts/src/gateway/HomeGatewayToEthereum.sol @@ -24,6 +24,7 @@ contract HomeGatewayToEthereum is IHomeGateway { mapping(uint256 => bytes32) public disputeIDtoHash; mapping(bytes32 => uint256) public disputeHashtoID; + address public governor; IArbitrator public arbitrator; IFastBridgeSender public fastbridge; address public foreignGateway; @@ -37,11 +38,13 @@ contract HomeGatewayToEthereum is IHomeGateway { mapping(bytes32 => RelayedData) public disputeHashtoRelayedData; constructor( + address _governor, IArbitrator _arbitrator, IFastBridgeSender _fastbridge, address _foreignGateway, uint256 _foreignChainID ) { + governor = _governor; arbitrator = _arbitrator; fastbridge = _fastbridge; foreignGateway = _foreignGateway; @@ -113,6 +116,14 @@ contract HomeGatewayToEthereum is IHomeGateway { fastbridge.sendFast(foreignGateway, data); } + /** @dev Changes the fastBridge, useful to increase the claim deposit. + * @param _fastbridge The address of the new fastBridge. + */ + function changeFastbridge(IFastBridgeSender _fastbridge) external { + require(governor == msg.sender, "Access not allowed: Governor only."); + fastbridge = _fastbridge; + } + function disputeHashToHomeID(bytes32 _disputeHash) external view returns (uint256) { return disputeHashtoID[_disputeHash]; } diff --git a/contracts/src/gateway/interfaces/IForeignGateway.sol b/contracts/src/gateway/interfaces/IForeignGateway.sol index a507b0ed3..1277dd435 100644 --- a/contracts/src/gateway/interfaces/IForeignGateway.sol +++ b/contracts/src/gateway/interfaces/IForeignGateway.sol @@ -11,6 +11,7 @@ interface IForeignGateway is IArbitrator { * Relay the rule call from the home gateway to the arbitrable. */ function relayRule( + address _messageOrigin, bytes32 _disputeHash, uint256 _ruling, address _forwarder