From 03414721fc94718f46d40cb15ea1f3e15a29b18f Mon Sep 17 00:00:00 2001 From: parodime Date: Thu, 17 Oct 2024 09:54:02 -0400 Subject: [PATCH 1/4] storage refs - readability & gas impv --- .../contracts-rfq/contracts/FastBridgeV2.sol | 68 ++++++++++--------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 270f962cf9..b718ceb7bb 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -77,18 +77,20 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { /// @inheritdoc IFastBridge function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) { - if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); - if (_timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD) { + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + + if ($.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); + if (_timeSince($.proofBlockTimestamp) > DISPUTE_PERIOD) { revert DisputePeriodPassed(); } - address disputedRelayer = bridgeTxDetails[transactionId].proofRelayer; + address disputedRelayer = $.proofRelayer; // @dev relayer gets slashed effectively if dest relay has gone thru - bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; - bridgeTxDetails[transactionId].proofRelayer = address(0); - bridgeTxDetails[transactionId].proofBlockTimestamp = 0; - bridgeTxDetails[transactionId].proofBlockNumber = 0; + $.status = BridgeStatus.REQUESTED; + $.proofRelayer = address(0); + $.proofBlockTimestamp = 0; + $.proofBlockNumber = 0; emit BridgeProofDisputed(transactionId, disputedRelayer); } @@ -99,7 +101,9 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); - if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + + if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); if (hasRole(REFUNDER_ROLE, msg.sender)) { // Refunder can refund if deadline has passed @@ -110,7 +114,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } // if all checks passed, set to REFUNDED status - bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; + $.status = BridgeStatus.REFUNDED; // transfer origin collateral back to original sender uint256 amount = transaction.originAmount + transaction.originFeeAmount; @@ -128,9 +132,11 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { /// @inheritdoc IFastBridge function canClaim(bytes32 transactionId, address relayer) external view returns (bool) { - if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); - if (bridgeTxDetails[transactionId].proofRelayer != relayer) revert SenderIncorrect(); - return _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD; + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + + if ($.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); + if ($.proofRelayer != relayer) revert SenderIncorrect(); + return _timeSince($.proofBlockTimestamp) > DISPUTE_PERIOD; } /// @inheritdoc IFastBridge @@ -273,12 +279,14 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { /// @inheritdoc IFastBridgeV2 function prove(bytes32 transactionId, bytes32 destTxHash, address relayer) public onlyRole(RELAYER_ROLE) { + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + // update bridge tx status given proof provided - if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); - bridgeTxDetails[transactionId].status = BridgeStatus.RELAYER_PROVED; - bridgeTxDetails[transactionId].proofBlockTimestamp = uint40(block.timestamp); - bridgeTxDetails[transactionId].proofBlockNumber = uint48(block.number); - bridgeTxDetails[transactionId].proofRelayer = relayer; + if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); + $.status = BridgeStatus.RELAYER_PROVED; + $.proofBlockTimestamp = uint40(block.timestamp); + $.proofBlockNumber = uint48(block.number); + $.proofRelayer = relayer; emit BridgeProofProvided(transactionId, relayer, destTxHash); } @@ -288,21 +296,23 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { bytes32 transactionId = keccak256(request); BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + // update bridge tx status if able to claim origin collateral - if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); + if ($.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); // if "to" is zero addr, permissionlessly send funds to proven relayer if (to == address(0)) { - to = bridgeTxDetails[transactionId].proofRelayer; - } else if (bridgeTxDetails[transactionId].proofRelayer != msg.sender) { + to = $.proofRelayer; + } else if ($.proofRelayer != msg.sender) { revert SenderIncorrect(); } - if (_timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) <= DISPUTE_PERIOD) { + if (_timeSince($.proofBlockTimestamp) <= DISPUTE_PERIOD) { revert DisputePeriodNotPassed(); } - bridgeTxDetails[transactionId].status = BridgeStatus.RELAYER_CLAIMED; + $.status = BridgeStatus.RELAYER_CLAIMED; // update protocol fees if origin fee amount exists if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount; @@ -317,13 +327,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { IERC20(token).safeTransfer(to, amount); } - emit BridgeDepositClaimed( - transactionId, - bridgeTxDetails[transactionId].proofRelayer, - to, - transaction.originToken, - transaction.originAmount - ); + emit BridgeDepositClaimed(transactionId, $.proofRelayer, to, transaction.originToken, transaction.originAmount); } function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { @@ -331,8 +335,10 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } function bridgeProofs(bytes32 transactionId) public view returns (uint96 timestamp, address relayer) { - timestamp = bridgeTxDetails[transactionId].proofBlockTimestamp; - relayer = bridgeTxDetails[transactionId].proofRelayer; + BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + + timestamp = $.proofBlockTimestamp; + relayer = $.proofRelayer; } /// @inheritdoc IFastBridgeV2 From b0b1fdd0f4c3f5b7610a42ea79d6467853ddfd42 Mon Sep 17 00:00:00 2001 From: parodime Date: Thu, 17 Oct 2024 13:13:22 -0400 Subject: [PATCH 2/4] rearrange reads on dispute --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index b718ceb7bb..36a482f57a 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -79,13 +79,15 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) { BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; - if ($.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); - if (_timeSince($.proofBlockTimestamp) > DISPUTE_PERIOD) { + address disputedRelayer = $.proofRelayer; + BridgeStatus status = $.status; + uint40 proofBlockTimestamp = $.proofBlockTimestamp; + + if (status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); + if (_timeSince(proofBlockTimestamp) > DISPUTE_PERIOD) { revert DisputePeriodPassed(); } - address disputedRelayer = $.proofRelayer; - // @dev relayer gets slashed effectively if dest relay has gone thru $.status = BridgeStatus.REQUESTED; $.proofRelayer = address(0); From d8b8e143d906be9c8d21d0cb8ad0f2938f12ab56 Mon Sep 17 00:00:00 2001 From: parodime Date: Thu, 17 Oct 2024 15:28:10 -0400 Subject: [PATCH 3/4] read storage to local stack - claim --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 36a482f57a..6e835c1fc1 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -300,17 +300,21 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; + address proofRelayer = $.proofRelayer; + BridgeStatus status = $.status; + uint40 proofBlockTimestamp = $.proofBlockTimestamp; + // update bridge tx status if able to claim origin collateral - if ($.status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); + if (status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); // if "to" is zero addr, permissionlessly send funds to proven relayer if (to == address(0)) { - to = $.proofRelayer; - } else if ($.proofRelayer != msg.sender) { + to = proofRelayer; + } else if (proofRelayer != msg.sender) { revert SenderIncorrect(); } - if (_timeSince($.proofBlockTimestamp) <= DISPUTE_PERIOD) { + if (_timeSince(proofBlockTimestamp) <= DISPUTE_PERIOD) { revert DisputePeriodNotPassed(); } @@ -329,7 +333,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { IERC20(token).safeTransfer(to, amount); } - emit BridgeDepositClaimed(transactionId, $.proofRelayer, to, transaction.originToken, transaction.originAmount); + emit BridgeDepositClaimed(transactionId, proofRelayer, to, transaction.originToken, transaction.originAmount); } function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { From d0be8f462343fae2879b4ce1b19e97e456411b85 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Fri, 18 Oct 2024 10:31:09 +0100 Subject: [PATCH 4/4] docs: make storage writes more explicit --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 6e835c1fc1..eb21a540e3 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -88,7 +88,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { revert DisputePeriodPassed(); } - // @dev relayer gets slashed effectively if dest relay has gone thru + // Note: these are storage writes $.status = BridgeStatus.REQUESTED; $.proofRelayer = address(0); $.proofBlockTimestamp = 0; @@ -115,7 +115,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); } - // if all checks passed, set to REFUNDED status + // Note: this is a storage write $.status = BridgeStatus.REFUNDED; // transfer origin collateral back to original sender @@ -285,6 +285,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { // update bridge tx status given proof provided if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); + // Note: these are storage writes $.status = BridgeStatus.RELAYER_PROVED; $.proofBlockTimestamp = uint40(block.timestamp); $.proofBlockNumber = uint48(block.number); @@ -317,7 +318,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { if (_timeSince(proofBlockTimestamp) <= DISPUTE_PERIOD) { revert DisputePeriodNotPassed(); } - + // Note: this is a storage write $.status = BridgeStatus.RELAYER_CLAIMED; // update protocol fees if origin fee amount exists