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): rework permisionless cancellation [SLT-489] #3382

Merged
merged 15 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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
68 changes: 68 additions & 0 deletions packages/contracts-rfq/contracts/AdminV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved
Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed

import {AccessControlEnumerable} from "@openzeppelin/contracts/access/extensions/AccessControlEnumerable.sol";

import {UniversalTokenLib} from "./libs/UniversalToken.sol";
import {IAdminV2} from "./interfaces/IAdminV2.sol";
import {IAdminV2Errors} from "./interfaces/IAdminV2Errors.sol";

contract AdminV2 is AccessControlEnumerable, IAdminV2, IAdminV2Errors {
using UniversalTokenLib for address;

bytes32 public constant RELAYER_ROLE = keccak256("RELAYER_ROLE");
bytes32 public constant CANCELER_ROLE = keccak256("CANCELER_ROLE");
bytes32 public constant GUARD_ROLE = keccak256("GUARD_ROLE");
bytes32 public constant GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE");

uint256 public constant FEE_BPS = 1e6;
uint256 public constant FEE_RATE_MAX = 0.01e6; // max 1% on origin amount
uint256 public constant MIN_CANCEL_DELAY = 1 hours;
uint256 public constant DEFAULT_CANCEL_DELAY = 1 days;

/// @notice Protocol fee rate taken on origin amount deposited in origin chain
uint256 public protocolFeeRate;

/// @notice Protocol fee amounts accumulated
mapping(address => uint256) public protocolFees;

/// @notice Delay for a transaction after which it could be permisionlessly cancelled
uint256 public cancelDelay;

/// @notice This is deprecated and should not be used.
/// @dev Use ZapNative V2 requests instead.
uint256 public immutable chainGasAmount = 0;
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

constructor(address _owner) {
_grantRole(DEFAULT_ADMIN_ROLE, _owner);
_setCancelDelay(DEFAULT_CANCEL_DELAY);
}
Comment on lines +60 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add zero-address validation for owner

The constructor should validate that _owner is not the zero address to prevent potential access control issues.

 constructor(address _owner) {
+    if (_owner == address(0)) revert InvalidOwner();
     _grantRole(DEFAULT_ADMIN_ROLE, _owner);
     _setCancelDelay(DEFAULT_CANCEL_DELAY);
 }

Committable suggestion skipped: line range outside the PR's diff.


function setCancelDelay(uint256 newCancelDelay) external onlyRole(GOVERNOR_ROLE) {
_setCancelDelay(newCancelDelay);
}

function setProtocolFeeRate(uint256 newFeeRate) external onlyRole(GOVERNOR_ROLE) {
if (newFeeRate > FEE_RATE_MAX) revert FeeRateAboveMax();
uint256 oldFeeRate = protocolFeeRate;
protocolFeeRate = newFeeRate;
emit FeeRateUpdated(oldFeeRate, newFeeRate);
}

function sweepProtocolFees(address token, address recipient) external onlyRole(GOVERNOR_ROLE) {
uint256 feeAmount = protocolFees[token];
if (feeAmount == 0) return; // skip if no accumulated fees

protocolFees[token] = 0;
token.universalTransfer(recipient, feeAmount);
emit FeesSwept(token, recipient, feeAmount);
}
Fixed Show fixed Hide fixed
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

/// @notice Internal function to set the cancel delay. Security checks are performed outside of this function.
function _setCancelDelay(uint256 newCancelDelay) private {
if (newCancelDelay < MIN_CANCEL_DELAY) revert CancelDelayBelowMin();
uint256 oldCancelDelay = cancelDelay;
cancelDelay = newCancelDelay;
emit CancelDelayUpdated(oldCancelDelay, newCancelDelay);
}
}
69 changes: 36 additions & 33 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

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

import {Admin} from "./Admin.sol";
import {AdminV2} from "./AdminV2.sol";
import {IFastBridge} from "./interfaces/IFastBridge.sol";
import {IFastBridgeV2} from "./interfaces/IFastBridgeV2.sol";
import {IFastBridgeV2Errors} from "./interfaces/IFastBridgeV2Errors.sol";
Expand All @@ -15,7 +15,7 @@
import {MulticallTarget} from "./utils/MulticallTarget.sol";

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

Expand All @@ -25,9 +25,6 @@
/// @notice Dispute period for relayed transactions
uint256 public constant DISPUTE_PERIOD = 30 minutes;

/// @notice Delay for a transaction after which it could be permisionlessly refunded
uint256 public constant REFUND_DELAY = 7 days;

/// @notice Minimum deadline period to relay a requested bridge transaction
uint256 public constant MIN_DEADLINE_PERIOD = 30 minutes;

Expand All @@ -47,7 +44,7 @@
/// @notice the block the contract was deployed at
uint256 public immutable deployBlock;

constructor(address _owner) Admin(_owner) {
constructor(address _owner) AdminV2(_owner) {
deployBlock = block.number;
}

Expand Down Expand Up @@ -104,33 +101,10 @@
emit BridgeProofDisputed(transactionId, disputedRelayer);
}

/// Note: this function is deprecated and will be removed in a future version.
/// @inheritdoc IFastBridge
function refund(bytes calldata request) external {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTxDetails storage $ = bridgeTxDetails[transactionId];
// Can only refund a REQUESTED transaction after its deadline expires
if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect();
uint256 deadline = request.deadline();
// Permissionless refund is only allowed after REFUND_DELAY on top of the deadline
if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY;
if (block.timestamp <= deadline) revert DeadlineNotExceeded();
// Update status to REFUNDED and return the full amount (collateral + protocol fees) to the original sender.
// The protocol fees are only updated when the transaction is claimed, so we don't need to update them here.
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;

address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
// Emit the event before any external calls
emit BridgeDepositRefunded(transactionId, to, token, amount);
// Complete the user refund as the last transaction action
if (token == NATIVE_GAS_TOKEN) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}
cancel(request);
}

/// @inheritdoc IFastBridge
Expand Down Expand Up @@ -363,6 +337,35 @@
}
}

/// @inheritdoc IFastBridgeV2
function cancel(bytes calldata request) public {
request.validateV2();
bytes32 transactionId = keccak256(request);
BridgeTxDetails storage $ = bridgeTxDetails[transactionId];
// Can only cancel a REQUESTED transaction after its deadline expires
if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect();
uint256 deadline = request.deadline();
// Permissionless cancel is only allowed after `cancelDelay` on top of the deadline
if (!hasRole(CANCELER_ROLE, msg.sender)) deadline += cancelDelay;
if (block.timestamp <= deadline) revert DeadlineNotExceeded();
// Update status to REFUNDED and return the full amount (collateral + protocol fees) to the original sender.
// The protocol fees are only updated when the transaction is claimed, so we don't need to update them here.
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;

address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
// Emit the event before any external calls
emit BridgeDepositRefunded(transactionId, to, token, amount);
// Complete the user cancel as the last transaction action
if (token == NATIVE_GAS_TOKEN) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}
}
Dismissed Show dismissed Hide dismissed
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

/// @inheritdoc IFastBridgeV2
function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) {
return bridgeTxDetails[transactionId].status;
Expand All @@ -383,8 +386,8 @@
}

/// @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.
/// claimed by the relayer who completed the relay on destination chain, or transferred back to the user
/// via the cancel function should no one complete the relay.
function _takeBridgedUserAsset(address token, uint256 amount) internal returns (uint256 amountTaken) {
if (token == NATIVE_GAS_TOKEN) {
// For the native gas token, we just need to check that the supplied msg.value is correct.
Expand Down
14 changes: 14 additions & 0 deletions packages/contracts-rfq/contracts/interfaces/IAdminV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;
Dismissed Show dismissed Hide dismissed
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved

interface IAdminV2 {
event CancelDelayUpdated(uint256 oldCancelDelay, uint256 newCancelDelay);
event FeeRateUpdated(uint256 oldFeeRate, uint256 newFeeRate);
event FeesSwept(address token, address recipient, uint256 amount);

function setCancelDelay(uint256 newCancelDelay) external;

function setProtocolFeeRate(uint256 newFeeRate) external;

function sweepProtocolFees(address token, address recipient) external;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

interface IAdminV2Errors {
error CancelDelayBelowMin();
error FeeRateAboveMax();
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ interface IFastBridgeV2 is IFastBridge {
/// @notice Can only send funds to the relayer address on the proof.
/// @param request The encoded bridge transaction to claim on origin chain
function claim(bytes memory request) external;

/// @notice Cancels an outstanding bridge transaction in case optimistic bridging failed and returns the full amount
/// to the original sender.
/// @param request The encoded bridge transaction to refund
function cancel(bytes memory request) external;

/// @notice Checks if a transaction has been relayed
/// @param transactionId The ID of the transaction to check
/// @return True if the transaction has been relayed, false otherwise
Expand Down
20 changes: 10 additions & 10 deletions packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -152,19 +152,19 @@ contract FastBridgeV2GasBenchmarkSrcTest is FastBridgeV2SrcBaseTest {
assertEq(srcToken.balanceOf(address(fastBridge)), initialFastBridgeBalanceToken);
}

function test_refundPermissioned_token() public {
function test_cancelPermissioned_token() public {
bytes32 txId = getTxId(bridgedTokenTx);
skipTimeAtLeast({time: DEADLINE});
refund({caller: refunder, bridgeTx: bridgedTokenTx});
cancel({caller: canceler, bridgeTx: bridgedTokenTx});
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REFUNDED);
assertEq(srcToken.balanceOf(userA), initialUserBalanceToken + tokenParams.originAmount);
assertEq(srcToken.balanceOf(address(fastBridge)), initialFastBridgeBalanceToken - tokenParams.originAmount);
}

function test_refundPermissionless_token() public {
function test_cancelPermissionless_token() public {
bytes32 txId = getTxId(bridgedTokenTx);
skipTimeAtLeast({time: DEADLINE + PERMISSIONLESS_REFUND_DELAY});
refund({caller: userB, bridgeTx: bridgedTokenTx});
skipTimeAtLeast({time: DEADLINE + PERMISSIONLESS_CANCEL_DELAY});
cancel({caller: userB, bridgeTx: bridgedTokenTx});
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REFUNDED);
assertEq(srcToken.balanceOf(userA), initialUserBalanceToken + tokenParams.originAmount);
assertEq(srcToken.balanceOf(address(fastBridge)), initialFastBridgeBalanceToken - tokenParams.originAmount);
Expand Down Expand Up @@ -236,19 +236,19 @@ contract FastBridgeV2GasBenchmarkSrcTest is FastBridgeV2SrcBaseTest {
assertEq(address(fastBridge).balance, initialFastBridgeBalanceEth);
}

function test_refundPermissioned_eth() public {
function test_cancelPermissioned_eth() public {
bytes32 txId = getTxId(bridgedEthTx);
skipTimeAtLeast({time: DEADLINE});
refund({caller: refunder, bridgeTx: bridgedEthTx});
cancel({caller: canceler, bridgeTx: bridgedEthTx});
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REFUNDED);
assertEq(userA.balance, initialUserBalanceEth + ethParams.originAmount);
assertEq(address(fastBridge).balance, initialFastBridgeBalanceEth - ethParams.originAmount);
}

function test_refundPermissionless_eth() public {
function test_cancelPermissionless_eth() public {
bytes32 txId = getTxId(bridgedEthTx);
skipTimeAtLeast({time: DEADLINE + PERMISSIONLESS_REFUND_DELAY});
refund({caller: userB, bridgeTx: bridgedEthTx});
skipTimeAtLeast({time: DEADLINE + PERMISSIONLESS_CANCEL_DELAY});
cancel({caller: userB, bridgeTx: bridgedEthTx});
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REFUNDED);
assertEq(userA.balance, initialUserBalanceEth + ethParams.originAmount);
assertEq(address(fastBridge).balance, initialFastBridgeBalanceEth - ethParams.originAmount);
Expand Down
Loading
Loading