feat(contracts/xapps): addressed rlusd audit findings (#3096)

The rlusd audit report was delivered and there were a variety of relevant findings that needed to be addressed: - Centralisation Risk For Bridge Address Configuration (OMRL-04) - Use A Caller-Determined Refund Recipient (OMRL-05) - No Mechanism For Deleting Routes (OMRL-06) - Missing Check For Routes (OMRL-07) - Unnecessary Approval For Lockbox (OMRL-09) - Pausing and Unpausing Have The Same Role (OMRL-11) - Optimising Redundant Condition Checks For lockbox_ (OMRL-12) We already addressed these primary findings in PRs #2985 and #3074: - Paused Token Leads to Loss of Funds (OMRL-01) - Insufficient Gas For receiveToken() (OMRL-02) Findings that are not relevant and do not need to be addressed are: - Risks Of Source And Destination Chain Configuration Mismatch (OMRL-03) - No Support For Rebasing Or Fee Tokens (OMRL-08) - New EVM Version May Be Unsupported (OMRL-10) issue: #3095
omni-network · Feb 19, 2025 · 205bbad · 205bbad
1 parent 4d6de89
commit 205bbad
Show file tree

Hide file tree

Showing 17 changed files with 621 additions and 163 deletions.
diff --git a/contracts/bindings/bridge.go b/contracts/bindings/bridge.go
diff --git a/contracts/bindings/lockbox.go b/contracts/bindings/lockbox.go
diff --git a/contracts/xapps/.gas-snapshot b/contracts/xapps/.gas-snapshot
@@ -1,29 +1,31 @@
 DepositTest:test_depositTo_other_succeeds() (gas: 153450)
 DepositTest:test_depositTo_self_succeeds() (gas: 142560)
-DepositTest:test_deposit_succeeds() (gas: 142215)
-DepositTest:test_deposits_reverts() (gas: 168237)
-GeneralBridgeTest:test_initialize_reverts() (gas: 3411964)
-GeneralBridgeTest:test_setRoutes_reverts() (gas: 66844)
-GeneralBridgeTest:test_setRoutes_succeeds() (gas: 47347)
-GeneralLockboxest:test_initialize_reverts() (gas: 1396931)
+DepositTest:test_deposit_succeeds() (gas: 142237)
+DepositTest:test_deposits_reverts() (gas: 172259)
+GeneralBridgeTest:test_authorizeRoutes_succeeds() (gas: 60040)
+GeneralBridgeTest:test_initialize_reverts() (gas: 3784860)
+GeneralBridgeTest:test_setRoutes_reverts() (gas: 56513)
+GeneralBridgeTest:test_setRoutes_succeeds() (gas: 47010)
+GeneralLockboxest:test_initialize_reverts() (gas: 1463389)
 PledgeTest:test_pledge_reverts() (gas: 22501)
 PledgeTest:test_pledge_success() (gas: 51984)
-ReceiveTokenTest:test_receiveToken_reverts() (gas: 135510)
-ReceiveTokenTest:test_receiveToken_succeeds_insolvent_lockbox() (gas: 193040)
-ReceiveTokenTest:test_receiveToken_succeeds_no_lockbox() (gas: 136444)
-ReceiveTokenTest:test_receiveToken_succeeds_paused() (gas: 300446)
-ReceiveTokenTest:test_receiveToken_succeeds_solvent_lockbox() (gas: 214028)
-RetryTransferTest:test_receiveToken_caches_tokens_when_mint_reverts() (gas: 210880)
-RetryTransferTest:test_retryTransfer_reverts() (gas: 116449)
-RetryTransferTest:test_retryTransfer_succeeds() (gas: 414477)
-SendTokenTest:test_sendToken_empty_lockbox_succeeds() (gas: 661109)
-SendTokenTest:test_sendToken_fee_overpayment_refunded() (gas: 372568)
-SendTokenTest:test_sendToken_reverts() (gas: 643995)
-SendTokenTest:test_sendToken_withLockbox_token_succeeds() (gas: 321729)
-SendTokenTest:test_sendToken_withLockbox_wrapper_succeeds() (gas: 301487)
-SendTokenTest:test_sendToken_withoutLockbox_wrapper_succeeds() (gas: 314544)
-SendTokenTest:test_sendToken_wrapper_overdrafted_lockbox_succeeds() (gas: 543610)
-WithdrawTest:test_withdrawTo_other_succeeds() (gas: 151100)
-WithdrawTest:test_withdrawTo_self_succeeds() (gas: 124918)
-WithdrawTest:test_withdraw_succeeds() (gas: 124780)
-WithdrawTest:test_withdraws_reverts() (gas: 173148)
+ReceiveTokenTest:test_receiveToken_reverts() (gas: 119272)
+ReceiveTokenTest:test_receiveToken_succeeds_insolvent_lockbox() (gas: 193199)
+ReceiveTokenTest:test_receiveToken_succeeds_no_lockbox() (gas: 136586)
+ReceiveTokenTest:test_receiveToken_succeeds_paused() (gas: 300659)
+ReceiveTokenTest:test_receiveToken_succeeds_solvent_lockbox() (gas: 214204)
+RetryTransferTest:test_receiveToken_caches_tokens_when_mint_reverts() (gas: 219978)
+RetryTransferTest:test_retryTransfer_reverts() (gas: 187338)
+RetryTransferTest:test_retryTransfer_succeeds() (gas: 418184)
+SendTokenTest:test_sendToken_empty_lockbox_succeeds() (gas: 662095)
+SendTokenTest:test_sendToken_fee_overpayment_refunded() (gas: 373086)
+SendTokenTest:test_sendToken_fee_overpayment_refunded_refundTo() (gas: 394480)
+SendTokenTest:test_sendToken_reverts() (gas: 663501)
+SendTokenTest:test_sendToken_withLockbox_token_succeeds() (gas: 322046)
+SendTokenTest:test_sendToken_withLockbox_wrapper_succeeds() (gas: 301839)
+SendTokenTest:test_sendToken_withoutLockbox_wrapper_succeeds() (gas: 314914)
+SendTokenTest:test_sendToken_wrapper_overdrafted_lockbox_succeeds() (gas: 544261)
+WithdrawTest:test_withdrawTo_other_succeeds() (gas: 151135)
+WithdrawTest:test_withdrawTo_self_succeeds() (gas: 124953)
+WithdrawTest:test_withdraw_succeeds() (gas: 124744)
+WithdrawTest:test_withdraws_reverts() (gas: 177058)
diff --git a/contracts/xapps/src/bridge/Bridge.sol b/contracts/xapps/src/bridge/Bridge.sol
@@ -22,6 +22,10 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
 
     // keccak256("PAUSER")
     bytes32 public constant PAUSER_ROLE = 0x539440820030c4994db4e31b6b800deafd503688728f932addfe7a410515c14c;
+    // keccak256("UNPAUSER");
+    bytes32 public constant UNPAUSER_ROLE = 0x82b32d9ab5100db08aeb9a0e08b422d14851ec118736590462bf9c085a6e9448;
+    // keccak256("AUTHORIZER");
+    bytes32 public constant AUTHORIZER_ROLE = 0x94858e5561d6a33fcce848f16acfe1514fe5166e32b456aff42d7fb50e8c52ad;
 
     /**
      * @dev Gas limit for xcalls to bridges without a lockbox. (~125k suggested unless transfer is nonstandard)
@@ -52,6 +56,11 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      */
     mapping(uint64 chainId => Route) private _routes;
 
+    /**
+     * @dev Mapping of destination chainId to pending route updates.
+     */
+    mapping(uint64 chainId => Route) private _pendingRoutes;
+
     /**
      * @dev Track claimable amount per address, which increases when `token` reverts in `receiveToken`.
      */
@@ -87,13 +96,20 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     /*                        INITIALIZER                         */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
-    function initialize(address admin_, address pauser_, address omni_, address token_, address lockbox_)
-        external
-        initializer
-    {
+    function initialize(
+        address admin_,
+        address authorizer_,
+        address pauser_,
+        address unpauser_,
+        address omni_,
+        address token_,
+        address lockbox_
+    ) external initializer {
         // Validate required inputs are not zero addresses.
         if (admin_ == address(0)) revert ZeroAddress();
+        if (authorizer_ == address(0)) revert ZeroAddress();
         if (pauser_ == address(0)) revert ZeroAddress();
+        if (unpauser_ == address(0)) revert ZeroAddress();
         if (omni_ == address(0)) revert ZeroAddress();
         if (token_ == address(0)) revert ZeroAddress();
 
@@ -103,15 +119,15 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
         __XApp_init(omni_, ConfLevel.Finalized);
         _grantRole(DEFAULT_ADMIN_ROLE, admin_);
         _grantRole(PAUSER_ROLE, pauser_);
+        _grantRole(UNPAUSER_ROLE, unpauser_);
+        _grantRole(AUTHORIZER_ROLE, authorizer_);
 
-        // Set configured values.
         token = token_;
-        if (lockbox_ != address(0)) lockbox = lockbox_;
 
         // Give lockbox relevant approvals to handle deposits and withdrawals if necessary.
         if (lockbox_ != address(0)) {
+            lockbox = lockbox_;
             ILockbox(lockbox_).token().safeApproveWithRetry(lockbox_, type(uint256).max);
-            token_.safeApproveWithRetry(lockbox_, type(uint256).max);
         }
     }
 
@@ -127,6 +143,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      */
     function getRoute(uint64 destChainId) external view returns (address bridge, bool hasLockbox) {
         Route memory route = _routes[destChainId];
+        if (route.bridge == address(0)) revert InvalidRoute(destChainId);
         return (route.bridge, route.hasLockbox);
     }
 
@@ -137,6 +154,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      */
     function bridgeFee(uint64 destChainId) external view returns (uint256 fee) {
         Route memory route = _routes[destChainId];
+        if (route.bridge == address(0)) revert InvalidRoute(destChainId);
         return feeFor(
             destChainId,
             abi.encodeCall(Bridge.receiveToken, (TypeMax.Address, TypeMax.Uint256)),
@@ -154,10 +172,15 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @param to          The address of the recipient.
      * @param value       The amount of tokens to bridge.
      * @param wrap        Whether to wrap the token first.
+     * @param refundTo    The address to refund any excess payment to.
      */
-    function sendToken(uint64 destChainId, address to, uint256 value, bool wrap) external payable whenNotPaused {
-        _validateSend(destChainId, to, value, wrap);
-        _sendToken(destChainId, to, value, wrap);
+    function sendToken(uint64 destChainId, address to, uint256 value, bool wrap, address refundTo)
+        external
+        payable
+        whenNotPaused
+    {
+        _validateSend(destChainId, to, value, wrap, refundTo);
+        _sendToken(destChainId, to, value, wrap, refundTo);
     }
 
     /**
@@ -173,7 +196,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @dev Attempts to transfer claimable tokens to the recipient.
      * @param addr The address to retry the transfer for.
      */
-    function claimFailedReceive(address addr) external {
+    function claimFailedReceive(address addr) external whenNotPaused {
         uint256 value = claimable[addr];
         if (value == 0) revert NoClaimable();
 
@@ -194,19 +217,34 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
     /**
-     * @dev Sets bridge routes for given chainIds.
+     * @dev Configures bridge routes for given chainIds.
      * @param chainIds The chainIds to configure.
      * @param routes   The bridges addresses and configs to configure.
      */
-    function setRoutes(uint64[] calldata chainIds, Route[] calldata routes) external onlyRole(DEFAULT_ADMIN_ROLE) {
+    function configureRoutes(uint64[] calldata chainIds, Route[] calldata routes)
+        external
+        onlyRole(DEFAULT_ADMIN_ROLE)
+    {
         if (chainIds.length != routes.length) revert ArrayLengthMismatch();
         for (uint256 i = 0; i < chainIds.length; i++) {
-            if (routes[i].bridge == address(0)) revert ZeroAddress();
-            _routes[chainIds[i]] = routes[i];
+            _pendingRoutes[chainIds[i]] = routes[i];
             emit RouteConfigured(chainIds[i], routes[i].bridge, routes[i].hasLockbox);
         }
     }
 
+    /**
+     * @dev Authorizes pending bridge routes.
+     * @param chainIds The chainIds for pending routes to authorize.
+     */
+    function authorizeRoutes(uint64[] calldata chainIds) external onlyRole(AUTHORIZER_ROLE) {
+        for (uint256 i = 0; i < chainIds.length; i++) {
+            Route memory pendingRoute = _pendingRoutes[chainIds[i]];
+            _routes[chainIds[i]] = pendingRoute;
+            delete _pendingRoutes[chainIds[i]];
+            emit RouteAuthorized(chainIds[i], pendingRoute.bridge, pendingRoute.hasLockbox);
+        }
+    }
+
     /**
      * @dev Pauses performing crosschain transfers.
      */
@@ -217,7 +255,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     /**
      * @dev Unpauses performing crosschain transfers.
      */
-    function unpause() external onlyRole(PAUSER_ROLE) {
+    function unpause() external onlyRole(UNPAUSER_ROLE) {
         _unpause();
     }
 
@@ -231,12 +269,14 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @param to          The address of the recipient.
      * @param value       The amount of tokens to transfer.
      * @param wrap        Whether the token is being wrapped.
+     * @param refundTo    The address to refund any excess payment to.
      */
-    function _validateSend(uint64 destChainId, address to, uint256 value, bool wrap) internal view {
+    function _validateSend(uint64 destChainId, address to, uint256 value, bool wrap, address refundTo) internal view {
         if (_routes[destChainId].bridge == address(0)) revert InvalidRoute(destChainId);
         if (to == address(0)) revert ZeroAddress();
         if (value == 0) revert ZeroAmount();
         if (wrap && lockbox == address(0)) revert CannotWrap();
+        if (refundTo == address(0)) revert ZeroAddress();
     }
 
     /**
@@ -245,8 +285,9 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @param to          The address of the recipient.
      * @param value       The amount of tokens to transfer.
      * @param wrap        Whether the token is being wrapped.
+     * @param refundTo    The address to refund any excess payment to.
      */
-    function _sendToken(uint64 destChainId, address to, uint256 value, bool wrap) internal {
+    function _sendToken(uint64 destChainId, address to, uint256 value, bool wrap, address refundTo) internal {
         // Cache addresses for gas optimization.
         address _token = token;
         address _lockbox = lockbox;
@@ -261,24 +302,25 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
         ITokenOps(_token).clawback(msg.sender, value);
 
         // Send the tokens to the destination chain.
-        _omniTransfer(destChainId, to, value);
+        _omniTransfer(destChainId, to, value, refundTo);
     }
 
     /**
      * @dev Handles the crosschain transfer of tokens.
      * @param destChainId The chainId of the destination chain.
      * @param to          The address of the recipient.
      * @param value       The amount of tokens to transfer.
+     * @param refundTo    The address to refund any excess payment to.
      */
-    function _omniTransfer(uint64 destChainId, address to, uint256 value) internal {
+    function _omniTransfer(uint64 destChainId, address to, uint256 value, address refundTo) internal {
         Route memory route = _routes[destChainId];
         bytes memory data = abi.encodeCall(Bridge.receiveToken, (to, value));
         uint256 fee = xcall(
             destChainId, route.bridge, data, route.hasLockbox ? RECEIVE_LOCKBOX_GAS_LIMIT : RECEIVE_DEFAULT_GAS_LIMIT
         );
 
         if (msg.value < fee) revert InsufficientPayment();
-        if (msg.value > fee) msg.sender.safeTransferETH(msg.value - fee);
+        if (msg.value > fee) refundTo.safeTransferETH(msg.value - fee);
 
         emit TokenSent(destChainId, msg.sender, to, value);
     }

diff --git a/contracts/xapps/src/bridge/Lockbox.sol b/contracts/xapps/src/bridge/Lockbox.sol
@@ -18,6 +18,8 @@ contract Lockbox is Initializable, AccessControlUpgradeable, PausableUpgradeable
 
     // keccak256("PAUSER")
     bytes32 public constant PAUSER_ROLE = 0x539440820030c4994db4e31b6b800deafd503688728f932addfe7a410515c14c;
+    // keccak256("UNPAUSER");
+    bytes32 public constant UNPAUSER_ROLE = 0x82b32d9ab5100db08aeb9a0e08b422d14851ec118736590462bf9c085a6e9448;
 
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
     /*                          STORAGE                           */
@@ -45,10 +47,14 @@ contract Lockbox is Initializable, AccessControlUpgradeable, PausableUpgradeable
     /*                        INITIALIZER                         */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
-    function initialize(address admin_, address pauser_, address token_, address wrapped_) external initializer {
+    function initialize(address admin_, address pauser_, address unpauser_, address token_, address wrapped_)
+        external
+        initializer
+    {
         // Validate required inputs are not zero addresses.
         if (admin_ == address(0)) revert ZeroAddress();
         if (pauser_ == address(0)) revert ZeroAddress();
+        if (unpauser_ == address(0)) revert ZeroAddress();
         if (token_ == address(0)) revert ZeroAddress();
         if (wrapped_ == address(0)) revert ZeroAddress();
 
@@ -57,6 +63,7 @@ contract Lockbox is Initializable, AccessControlUpgradeable, PausableUpgradeable
         __Pausable_init();
         _grantRole(DEFAULT_ADMIN_ROLE, admin_);
         _grantRole(PAUSER_ROLE, pauser_);
+        _grantRole(UNPAUSER_ROLE, unpauser_);
 
         // Set configured values.
         token = token_;
@@ -115,7 +122,7 @@ contract Lockbox is Initializable, AccessControlUpgradeable, PausableUpgradeable
     /**
      * @dev Unpauses depositing into and withdrawing from the lockbox.
      */
-    function unpause() external onlyRole(PAUSER_ROLE) {
+    function unpause() external onlyRole(UNPAUSER_ROLE) {
         _unpause();
     }
 

diff --git a/contracts/xapps/src/bridge/interfaces/IBridge.sol b/contracts/xapps/src/bridge/interfaces/IBridge.sol
@@ -55,6 +55,11 @@ interface IBridge {
      */
     event RouteConfigured(uint64 indexed destChainId, address indexed bridge, bool indexed hasLockbox);
 
+    /**
+     * @dev Event emitted when a bridge route is authorized.
+     */
+    event RouteAuthorized(uint64 indexed destChainId, address indexed bridge, bool indexed hasLockbox);
+
     /**
      * @dev Event emitted when a crosschain token transfer is initiated.
      */
@@ -133,8 +138,9 @@ interface IBridge {
      * @param to          The address of the recipient.
      * @param value       The amount of tokens to bridge.
      * @param wrap        Whether to wrap the token first.
+     * @param refundTo    The address to refund any excess payment to.
      */
-    function sendToken(uint64 destChainId, address to, uint256 value, bool wrap) external payable;
+    function sendToken(uint64 destChainId, address to, uint256 value, bool wrap, address refundTo) external payable;
 
     /**
      * @dev Receives a token from a bridge contract and mints/unwraps it to the recipient.
@@ -158,5 +164,11 @@ interface IBridge {
      * @param chainIds The chainIds to configure.
      * @param routes   The bridges addresses and configs to configure.
      */
-    function setRoutes(uint64[] calldata chainIds, Route[] calldata routes) external;
+    function configureRoutes(uint64[] calldata chainIds, Route[] calldata routes) external;
+
+    /**
+     * @dev Authorizes pending bridge routes.
+     * @param chainIds The chainIds for pending routes to authorize.
+     */
+    function authorizeRoutes(uint64[] calldata chainIds) external;
 }