Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(contracts-rfq): add version to BridgeTransaction, tight packing [SLT-328] [SLT-273] #3284

Merged
merged 15 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 53 additions & 65 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";

import {BridgeTransactionV2Lib} from "./libs/BridgeTransactionV2.sol";
import {UniversalTokenLib} from "./libs/UniversalToken.sol";

import {Admin} from "./Admin.sol";
Expand All @@ -14,6 +15,7 @@

/// @notice FastBridgeV2 is a contract for bridging tokens across chains.
contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
using BridgeTransactionV2Lib for bytes;
using SafeERC20 for IERC20;
using UniversalTokenLib for address;

Expand Down Expand Up @@ -61,17 +63,20 @@
}

/// @inheritdoc IFastBridge
function relay(bytes memory request) external payable {
function relay(bytes calldata request) external payable {
// relay override will validate the request
relay({request: request, relayer: msg.sender});
}

/// @inheritdoc IFastBridge
function prove(bytes memory request, bytes32 destTxHash) external {
function prove(bytes calldata request, bytes32 destTxHash) external {
request.validateV2();
prove({transactionId: keccak256(request), destTxHash: destTxHash, relayer: msg.sender});
}

/// @inheritdoc IFastBridgeV2
function claim(bytes memory request) external {
function claim(bytes calldata request) external {
// claim override will validate the request
claim({request: request, to: address(0)});
}

Expand All @@ -98,39 +103,32 @@
}

/// @inheritdoc IFastBridge
function refund(bytes memory request) external {
function refund(bytes calldata request) external {
request.validateV2();
bytes32 transactionId = keccak256(request);

BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request);

BridgeTxDetails storage $ = bridgeTxDetails[transactionId];

if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect();

if (hasRole(REFUNDER_ROLE, msg.sender)) {
// Refunder can refund if deadline has passed
if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded();
} else {
// Permissionless refund is allowed after REFUND_DELAY
if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded();
}

uint256 deadline = request.deadline();
// Permissionless refund is allowed after REFUND_DELAY
if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY;
if (block.timestamp <= deadline) revert DeadlineNotExceeded();
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;

// transfer origin collateral back to original sender
uint256 amount = transaction.originAmount + transaction.originFeeAmount;
address to = transaction.originSender;
address token = transaction.originToken;

address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
if (token == UniversalTokenLib.ETH_ADDRESS) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}

emit BridgeDepositRefunded(transactionId, transaction.originSender, transaction.originToken, amount);
emit BridgeDepositRefunded(transactionId, to, token, amount);
Comment on lines +106 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential Reentrancy Vulnerability in refund Function

The refund function may be susceptible to reentrancy attacks due to external calls to Address.sendValue or IERC20(token).safeTransfer before emitting the BridgeDepositRefunded event. To mitigate this risk, it's best practice to update the state and emit events before making external calls.

Apply this diff to move the event emission before the external calls:

 // Note: this is a storage write
 $.status = BridgeStatus.REFUNDED;

+emit BridgeDepositRefunded(transactionId, to, token, amount);

 // transfer origin collateral back to original sender
 address to = request.originSender();
 address token = request.originToken();
 uint256 amount = request.originAmount() + request.originFeeAmount();
 if (token == UniversalTokenLib.ETH_ADDRESS) {
     Address.sendValue(payable(to), amount);
 } else {
     IERC20(token).safeTransfer(to, amount);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function refund(bytes calldata request) external {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request);
BridgeTxDetails storage $ = bridgeTxDetails[transactionId];
if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect();
if (hasRole(REFUNDER_ROLE, msg.sender)) {
// Refunder can refund if deadline has passed
if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded();
} else {
// Permissionless refund is allowed after REFUND_DELAY
if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded();
}
uint256 deadline = request.deadline();
// Permissionless refund is allowed after REFUND_DELAY
if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY;
if (block.timestamp <= deadline) revert DeadlineNotExceeded();
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;
// transfer origin collateral back to original sender
uint256 amount = transaction.originAmount + transaction.originFeeAmount;
address to = transaction.originSender;
address token = transaction.originToken;
address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
if (token == UniversalTokenLib.ETH_ADDRESS) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}
emit BridgeDepositRefunded(transactionId, transaction.originSender, transaction.originToken, amount);
emit BridgeDepositRefunded(transactionId, to, token, amount);
function refund(bytes calldata request) external {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTxDetails storage $ = bridgeTxDetails[transactionId];
if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect();
uint256 deadline = request.deadline();
// Permissionless refund is allowed after REFUND_DELAY
if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY;
if (block.timestamp <= deadline) revert DeadlineNotExceeded();
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;
emit BridgeDepositRefunded(transactionId, to, token, amount);
// transfer origin collateral back to original sender
address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
if (token == UniversalTokenLib.ETH_ADDRESS) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}
🧰 Tools
🪛 GitHub Check: Slither

[notice] 106-131: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#125)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#127)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#130)


[notice] 106-131: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#116)

}

Check notice

Code scanning / Slither

Reentrancy vulnerabilities Low

Check notice

Code scanning / Slither

Block timestamp Low

FastBridgeV2.refund(bytes) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline

/// @inheritdoc IFastBridge
function canClaim(bytes32 transactionId, address relayer) external view returns (bool) {
Expand All @@ -146,7 +144,7 @@
/// - `callValue` is partially reported as a zero/non-zero flag
/// - `callParams` is ignored
/// In order to process all kinds of requests use getBridgeTransactionV2 instead.
function getBridgeTransaction(bytes memory request) external view returns (BridgeTransaction memory) {
function getBridgeTransaction(bytes calldata request) external view returns (BridgeTransaction memory) {
// Try decoding into V2 struct first. This will revert if V1 struct is passed
try this.getBridgeTransactionV2(request) returns (BridgeTransactionV2 memory txV2) {
// Note: we entirely ignore the callParams field, as it was not present in V1
Expand All @@ -170,6 +168,12 @@
}
}

/// @inheritdoc IFastBridgeV2
function getBridgeTransactionV2(bytes calldata request) external pure returns (BridgeTransactionV2 memory) {
request.validateV2();
return BridgeTransactionV2Lib.decodeV2(request);
}

/// @inheritdoc IFastBridgeV2
function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) public payable {
int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds;
Expand All @@ -187,7 +191,7 @@
}

// set status to requested
bytes memory request = abi.encode(
bytes memory request = BridgeTransactionV2Lib.encodeV2(
BridgeTransactionV2({
originChainId: uint32(block.chainid),
destChainId: params.dstChainId,
Expand All @@ -198,12 +202,12 @@
originAmount: originAmount,
destAmount: params.destAmount,
originFeeAmount: originFeeAmount,
callValue: paramsV2.callValue,
deadline: params.deadline,
nonce: senderNonces[params.sender]++, // increment nonce on every bridge
exclusivityRelayer: paramsV2.quoteRelayer,
// We checked exclusivityEndTime to be in range (0 .. params.deadline] above, so can safely cast
exclusivityEndTime: uint256(exclusivityEndTime),
callValue: paramsV2.callValue,
callParams: paramsV2.callParams
})
);
Expand All @@ -225,29 +229,29 @@
}

/// @inheritdoc IFastBridgeV2
function relay(bytes memory request, address relayer) public payable {
function relay(bytes calldata request, address relayer) public payable {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request);
_validateRelayParams(transaction, transactionId, relayer);
_validateRelayParams(request, transactionId, relayer);
// mark bridge transaction as relayed
bridgeRelayDetails[transactionId] =
BridgeRelay({blockNumber: uint48(block.number), blockTimestamp: uint48(block.timestamp), relayer: relayer});

// transfer tokens to recipient on destination chain and do an arbitrary call if requested
address to = transaction.destRecipient;
address token = transaction.destToken;
uint256 amount = transaction.destAmount;
uint256 callValue = transaction.callValue;
address to = request.destRecipient();
address token = request.destToken();
uint256 amount = request.destAmount();
uint256 callValue = request.callValue();

// Emit the event before any external calls
emit BridgeRelayed({
transactionId: transactionId,
relayer: relayer,
to: to,
originChainId: transaction.originChainId,
originToken: transaction.originToken,
originChainId: request.originChainId(),
originToken: request.originToken(),
destToken: token,
originAmount: transaction.originAmount,
originAmount: request.originAmount(),
destAmount: amount,
chainGasAmount: callValue
});
Expand All @@ -267,11 +271,12 @@
IERC20(token).safeTransferFrom(msg.sender, to, amount);
}

if (transaction.callParams.length != 0) {
bytes calldata callParams = request.callParams();
if (callParams.length != 0) {
// Arbitrary call requested, perform it while supplying full msg.value to the recipient
// Note: if token has a fee on transfers, the recipient will have received less than `amount`.
// This is a very niche edge case and should be handled by the recipient contract.
_checkedCallRecipient({recipient: to, token: token, amount: amount, callParams: transaction.callParams});
_checkedCallRecipient({recipient: to, token: token, amount: amount, callParams: callParams});
} else if (msg.value != 0) {
// No arbitrary call requested, but msg.value was sent. This is either a relay with ETH,
// or a non-zero callValue request with an ERC20. In both cases, transfer the ETH to the recipient.
Expand All @@ -295,47 +300,46 @@
}

/// @inheritdoc IFastBridge
function claim(bytes memory request, address to) public {
function claim(bytes calldata request, address to) public {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request);

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 "to" is zero addr, permissionlessly send funds to proven relayer
if (to == address(0)) {
to = proofRelayer;
} else if (proofRelayer != msg.sender) {
revert SenderIncorrect();
}

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
if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount;

address token = transaction.originToken;
uint256 amount = transaction.originAmount;
address token = request.originToken();
uint256 amount = request.originAmount();
uint256 originFeeAmount = request.originFeeAmount();
if (originFeeAmount > 0) protocolFees[token] += originFeeAmount;

// transfer origin collateral to specified address (protocol fee was pre-deducted at deposit)
if (token == UniversalTokenLib.ETH_ADDRESS) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}

emit BridgeDepositClaimed(transactionId, proofRelayer, to, transaction.originToken, transaction.originAmount);
emit BridgeDepositClaimed(transactionId, proofRelayer, to, token, amount);
}

Check notice

Code scanning / Slither

Reentrancy vulnerabilities Low

Check notice

Code scanning / Slither

Block timestamp Low

FastBridgeV2.claim(bytes,address) uses timestamp for comparisons
Dangerous comparisons:
- _timeSince(proofBlockTimestamp) <= DISPUTE_PERIOD

function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) {
return bridgeTxDetails[transactionId].status;
Expand All @@ -354,11 +358,6 @@
return bridgeRelayDetails[transactionId].relayer != address(0);
}

/// @inheritdoc IFastBridgeV2
function getBridgeTransactionV2(bytes memory request) public pure returns (BridgeTransactionV2 memory) {
return abi.decode(request, (BridgeTransactionV2));
}

/// @notice Takes the bridged asset from the user into FastBridgeV2 custody. It will be later
/// claimed by the relayer who completed the relay on destination chain, or refunded back to the user,
/// should no one complete the relay.
Expand Down Expand Up @@ -386,7 +385,7 @@
address recipient,
address token,
uint256 amount,
bytes memory callParams
bytes calldata callParams
)
internal
{
Expand Down Expand Up @@ -446,28 +445,17 @@
}

/// @notice Performs all the necessary checks for a relay to happen.
function _validateRelayParams(
BridgeTransactionV2 memory transaction,
bytes32 transactionId,
address relayer
)
internal
view
{
function _validateRelayParams(bytes calldata request, bytes32 transactionId, address relayer) internal view {
if (relayer == address(0)) revert ZeroAddress();
// Check if the transaction has already been relayed
if (bridgeRelays(transactionId)) revert TransactionRelayed();
if (transaction.destChainId != block.chainid) revert ChainIncorrect();
if (request.destChainId() != block.chainid) revert ChainIncorrect();
// Check the deadline for relay to happen
if (block.timestamp > transaction.deadline) revert DeadlineExceeded();
if (block.timestamp > request.deadline()) revert DeadlineExceeded();
// Check the exclusivity period, if it is still ongoing
// forgefmt: disable-next-item
if (
transaction.exclusivityRelayer != address(0) &&
transaction.exclusivityRelayer != relayer &&
block.timestamp <= transaction.exclusivityEndTime
) {
address exclRelayer = request.exclusivityRelayer();
if (exclRelayer != address(0) && exclRelayer != relayer && block.timestamp <= request.exclusivityEndTime()) {
revert ExclusivityPeriodNotPassed();
}
}
}
Loading
Loading