feat(contracts): applied xbridge audit feedback

contracts/xapps/.gas-snapshot

@@ -1,31 +1,31 @@
-DepositTest:test_depositTo_other_succeeds() (gas: 153450)
-DepositTest:test_depositTo_self_succeeds() (gas: 142560)
-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: 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)
+DepositTest:test_depositTo_other_succeeds() (gas: 152875)
+DepositTest:test_depositTo_self_succeeds() (gas: 142189)
+DepositTest:test_deposit_succeeds() (gas: 141866)
+DepositTest:test_deposits_reverts() (gas: 171559)
+GeneralBridgeTest:test_authorizeRoutes_succeeds() (gas: 62401)
+GeneralBridgeTest:test_initialize_reverts() (gas: 3886191)
+GeneralBridgeTest:test_setRoutes_reverts() (gas: 56280)
+GeneralBridgeTest:test_setRoutes_succeeds() (gas: 46763)
+GeneralLockboxest:test_initialize_reverts() (gas: 1429271)
+PledgeTest:test_pledge_reverts() (gas: 22389)
+PledgeTest:test_pledge_success() (gas: 51888)
+ReceiveTokenTest:test_receiveToken_reverts() (gas: 117997)
+ReceiveTokenTest:test_receiveToken_succeeds_insolvent_lockbox() (gas: 191348)
+ReceiveTokenTest:test_receiveToken_succeeds_no_lockbox() (gas: 136032)
+ReceiveTokenTest:test_receiveToken_succeeds_paused() (gas: 298562)
+ReceiveTokenTest:test_receiveToken_succeeds_solvent_lockbox() (gas: 213575)
+RetryTransferTest:test_receiveToken_caches_tokens_when_mint_reverts() (gas: 219269)
+RetryTransferTest:test_retryTransfer_reverts() (gas: 186870)
+RetryTransferTest:test_retryTransfer_succeeds() (gas: 417363)
+SendTokenTest:test_sendToken_empty_lockbox_succeeds() (gas: 656819)
+SendTokenTest:test_sendToken_fee_overpayment_refunded() (gas: 371367)
+SendTokenTest:test_sendToken_fee_overpayment_refunded_refundTo() (gas: 392760)
+SendTokenTest:test_sendToken_reverts() (gas: 658906)
+SendTokenTest:test_sendToken_withLockbox_token_succeeds() (gas: 320775)
+SendTokenTest:test_sendToken_withLockbox_wrapper_succeeds() (gas: 300349)
+SendTokenTest:test_sendToken_withoutLockbox_wrapper_succeeds() (gas: 313398)
+SendTokenTest:test_sendToken_wrapper_overdrafted_lockbox_succeeds() (gas: 540208)
+WithdrawTest:test_withdrawTo_other_succeeds() (gas: 150629)
+WithdrawTest:test_withdrawTo_self_succeeds() (gas: 124611)
+WithdrawTest:test_withdraw_succeeds() (gas: 124401)
+WithdrawTest:test_withdraws_reverts() (gas: 176350)

contracts/xapps/src/bridge/Bridge.sol

@@ -12,6 +12,7 @@ import { TypeMax } from "core/src/libraries/TypeMax.sol";
 import { SafeTransferLib } from "solady/src/utils/SafeTransferLib.sol";
 import { ITokenOps } from "./interfaces/ITokenOps.sol";
 import { ILockbox } from "./interfaces/ILockbox.sol";
+import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol";
 
 contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable, XAppUpgradeable, IBridge {
     using SafeTransferLib for address;
@@ -20,10 +21,12 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     /*                         CONSTANTS                          */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
-    // keccak256("PAUSER")
+    // keccak256("PAUSER");
     bytes32 public constant PAUSER_ROLE = 0x539440820030c4994db4e31b6b800deafd503688728f932addfe7a410515c14c;
     // keccak256("UNPAUSER");
     bytes32 public constant UNPAUSER_ROLE = 0x82b32d9ab5100db08aeb9a0e08b422d14851ec118736590462bf9c085a6e9448;
+    // keccak256("CONFIGURER");
+    bytes32 public constant CONFIGURER_ROLE = 0x527e2c92bb6983874717bce74818faf5a9be45b6e85909ee478af653c6d98755;
     // keccak256("AUTHORIZER");
     bytes32 public constant AUTHORIZER_ROLE = 0x94858e5561d6a33fcce848f16acfe1514fe5166e32b456aff42d7fb50e8c52ad;
 
@@ -98,6 +101,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
 
     function initialize(
         address admin_,
+        address configurer_,
         address authorizer_,
         address pauser_,
         address unpauser_,
@@ -107,6 +111,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     ) external initializer {
         // Validate required inputs are not zero addresses.
         if (admin_ == address(0)) revert ZeroAddress();
+        if (configurer_ == address(0)) revert ZeroAddress();
         if (authorizer_ == address(0)) revert ZeroAddress();
         if (pauser_ == address(0)) revert ZeroAddress();
         if (unpauser_ == address(0)) revert ZeroAddress();
@@ -118,10 +123,10 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
         __Pausable_init();
         __XApp_init(omni_, ConfLevel.Finalized);
         _grantRole(DEFAULT_ADMIN_ROLE, admin_);
+        _grantRole(CONFIGURER_ROLE, configurer_);
+        _grantRole(AUTHORIZER_ROLE, authorizer_);
         _grantRole(PAUSER_ROLE, pauser_);
         _grantRole(UNPAUSER_ROLE, unpauser_);
-        _grantRole(AUTHORIZER_ROLE, authorizer_);
-
         token = token_;
 
         // Give lockbox relevant approvals to handle deposits and withdrawals if necessary.
@@ -221,10 +226,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @param chainIds The chainIds to configure.
      * @param routes   The bridges addresses and configs to configure.
      */
-    function configureRoutes(uint64[] calldata chainIds, Route[] calldata routes)
-        external
-        onlyRole(DEFAULT_ADMIN_ROLE)
-    {
+    function configureRoutes(uint64[] calldata chainIds, Route[] calldata routes) external onlyRole(CONFIGURER_ROLE) {
         if (chainIds.length != routes.length) revert ArrayLengthMismatch();
         for (uint256 i = 0; i < chainIds.length; i++) {
             _pendingRoutes[chainIds[i]] = routes[i];
@@ -233,12 +235,20 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     }
 
     /**
-     * @dev Authorizes pending bridge routes.
-     * @param chainIds The chainIds for pending routes to authorize.
+     * @dev Authorizes pending bridge routes, manual specification prevents frontrunning.
+     * @param chainIds       The chainIds for pending routes to authorize.
+     * @param expectedRoutes The expected routes to authorize.
      */
-    function authorizeRoutes(uint64[] calldata chainIds) external onlyRole(AUTHORIZER_ROLE) {
+    function authorizeRoutes(uint64[] calldata chainIds, Route[] calldata expectedRoutes)
+        external
+        onlyRole(AUTHORIZER_ROLE)
+    {
         for (uint256 i = 0; i < chainIds.length; i++) {
             Route memory pendingRoute = _pendingRoutes[chainIds[i]];
+            if (
+                pendingRoute.bridge != expectedRoutes[i].bridge
+                    || pendingRoute.hasLockbox != expectedRoutes[i].hasLockbox
+            ) revert InvalidRoute(chainIds[i]);
             _routes[chainIds[i]] = pendingRoute;
             delete _pendingRoutes[chainIds[i]];
             emit RouteAuthorized(chainIds[i], pendingRoute.bridge, pendingRoute.hasLockbox);
@@ -343,7 +353,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
             // If a lockbox is set, mint the wrapped tokens to the bridge contract.
             success = _tryCatchTokenMint(_token, to, value, true);
             // Attempt withdrawal from the lockbox, but transfer the wrapped tokens to the recipient if it fails.
-            if (success) _tryCatchLockboxWithdrawal(_token, _lockbox, to, value);
+            if (success) success = _tryCatchLockboxWithdrawal(_token, _lockbox, to, value);
         }
 
         emit TokenReceived(xmsg.sourceChainId, to, value, success);
@@ -359,9 +369,7 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
     function _tryCatchTokenMint(address _token, address to, uint256 value, bool intermediate) internal returns (bool) {
         try ITokenOps(_token).mint(intermediate ? address(this) : to, value) { }
         catch {
-            unchecked {
-                claimable[to] += value;
-            }
+            _incrementClaimable(to, value);
             emit TokenMintFailed(_token, to, value);
             return false;
         }
@@ -375,11 +383,30 @@ contract Bridge is Initializable, AccessControlUpgradeable, PausableUpgradeable,
      * @param to        The address of the recipient.
      * @param value     The amount of tokens to withdraw.
      */
-    function _tryCatchLockboxWithdrawal(address _token, address _lockbox, address to, uint256 value) internal {
+    function _tryCatchLockboxWithdrawal(address _token, address _lockbox, address to, uint256 value)
+        internal
+        returns (bool)
+    {
         try ILockbox(_lockbox).withdrawTo(to, value) { }
         catch {
-            _token.safeTransfer(to, value);
-            emit LockboxWithdrawalFailed(_lockbox, to, value);
+            try IERC20(_token).transfer(to, value) returns (bool success) {
+                if (!success) {
+                    _incrementClaimable(to, value);
+                    emit TokenTransferFailed(_token, to, value);
+                    return false;
+                }
+            } catch {
+                _incrementClaimable(to, value);
+                emit LockboxWithdrawalFailed(_lockbox, to, value);
+                return false;
+            }
+        }
+        return true;
+    }
+
+    function _incrementClaimable(address to, uint256 value) internal {
+        unchecked {
+            claimable[to] += value;
         }
     }
 }

contracts/xapps/src/bridge/interfaces/IBridge.sol

@@ -80,6 +80,11 @@ interface IBridge {
      */
     event TokenMintFailed(address indexed token, address indexed to, uint256 value);
 
+    /**
+     * @dev Event emitted when a token transfer fails.
+     */
+    event TokenTransferFailed(address indexed token, address indexed to, uint256 value);
+
     /**
      * @dev Event emitted when a lockbox withdrawal fails.
      */
@@ -167,8 +172,9 @@ interface IBridge {
     function configureRoutes(uint64[] calldata chainIds, Route[] calldata routes) external;
 
     /**
-     * @dev Authorizes pending bridge routes.
-     * @param chainIds The chainIds for pending routes to authorize.
+     * @dev Authorizes pending bridge routes, manual specification prevents frontrunning.
+     * @param chainIds       The chainIds for pending routes to authorize.
+     * @param expectedRoutes The expected routes to authorize.
      */
-    function authorizeRoutes(uint64[] calldata chainIds) external;
+    function authorizeRoutes(uint64[] calldata chainIds, Route[] calldata expectedRoutes) external;
 }

contracts/xapps/test/bridge/TestBase.sol

@@ -27,8 +27,8 @@ contract TestBase is Test {
 
     uint64 internal constant SRC_CHAIN_ID = 1;
     uint64 internal constant DEST_CHAIN_ID = 2;
-    uint64 internal constant DEFAULT_RECEIVE_GAS_LIMIT = 125_000;
-    uint64 internal constant DEFAULT_RECEIVE_LOCKBOX_GAS_LIMIT = 200_000;
+    uint64 internal constant DEFAULT_RECEIVE_GAS_LIMIT = 150_000;
+    uint64 internal constant DEFAULT_RECEIVE_LOCKBOX_GAS_LIMIT = 250_000;
     uint256 internal constant INITIAL_USER_BALANCE = 1_000_000 ether;
 
     address internal user = makeAddr("user");
@@ -39,6 +39,7 @@ contract TestBase is Test {
     address internal unpauser = makeAddr("unpauser");
     address internal upgrader = makeAddr("upgrader");
     address internal clawbacker = makeAddr("clawbacker");
+    address internal configurer = makeAddr("configurer");
     address internal authorizer = makeAddr("authorizer");
 
     modifier prank(address addr) {
@@ -150,8 +151,9 @@ contract TestBase is Test {
 
     function _deployBridge(address token_, address lockbox_) internal returns (Bridge) {
         address impl = address(new Bridge(DEFAULT_RECEIVE_GAS_LIMIT, DEFAULT_RECEIVE_LOCKBOX_GAS_LIMIT));
-        bytes memory data =
-            abi.encodeCall(Bridge.initialize, (admin, authorizer, pauser, unpauser, address(omni), token_, lockbox_));
+        bytes memory data = abi.encodeCall(
+            Bridge.initialize, (admin, configurer, authorizer, pauser, unpauser, address(omni), token_, lockbox_)
+        );
 
         address proxy = address(new TransparentUpgradeableProxy(impl, admin, data));
         return Bridge(proxy);
@@ -189,17 +191,17 @@ contract TestBase is Test {
 
         chainIds[0] = DEST_CHAIN_ID;
         routes[0] = IBridge.Route({ bridge: address(bridgeNoLockbox), hasLockbox: false });
-        vm.prank(admin);
+        vm.prank(configurer);
         bridgeWithLockbox.configureRoutes(chainIds, routes);
         vm.prank(authorizer);
-        bridgeWithLockbox.authorizeRoutes(chainIds);
+        bridgeWithLockbox.authorizeRoutes(chainIds, routes);
 
         chainIds[0] = SRC_CHAIN_ID;
         routes[0] = IBridge.Route({ bridge: address(bridgeWithLockbox), hasLockbox: true });
-        vm.prank(admin);
+        vm.prank(configurer);
         bridgeNoLockbox.configureRoutes(chainIds, routes);
         vm.prank(authorizer);
-        bridgeNoLockbox.authorizeRoutes(chainIds);
+        bridgeNoLockbox.authorizeRoutes(chainIds, routes);
     }
 
     function _configurePermissions() internal {

contracts/xapps/test/bridge/bridge/General.t.sol

@@ -12,6 +12,20 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: address(0),
+            configurer_: address(0),
+            authorizer_: address(0),
+            pauser_: address(0),
+            unpauser_: address(0),
+            omni_: address(0),
+            token_: address(0),
+            lockbox_: address(0)
+        });
+
+        // `configurer` cannot be zero address.
+        vm.expectRevert(IBridge.ZeroAddress.selector);
+        bridgeWithLockbox.initialize({
+            admin_: admin,
+            configurer_: address(0),
             authorizer_: address(0),
             pauser_: address(0),
             unpauser_: address(0),
@@ -24,6 +38,7 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: address(0),
             pauser_: address(0),
             unpauser_: address(0),
@@ -36,6 +51,7 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: address(0),
             unpauser_: address(0),
@@ -48,6 +64,7 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: pauser,
             unpauser_: address(0),
@@ -60,6 +77,7 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: pauser,
             unpauser_: unpauser,
@@ -72,6 +90,7 @@ contract GeneralBridgeTest is TestBase {
         vm.expectRevert(IBridge.ZeroAddress.selector);
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: pauser,
             unpauser_: unpauser,
@@ -83,6 +102,7 @@ contract GeneralBridgeTest is TestBase {
         // Initialization works with all fields populated.
         bridgeWithLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: pauser,
             unpauser_: unpauser,
@@ -95,6 +115,7 @@ contract GeneralBridgeTest is TestBase {
         bridgeNoLockbox = Bridge(address(new TransparentUpgradeableProxy(impl, admin, "")));
         bridgeNoLockbox.initialize({
             admin_: admin,
+            configurer_: configurer,
             authorizer_: authorizer,
             pauser_: pauser,
             unpauser_: unpauser,
@@ -104,7 +125,7 @@ contract GeneralBridgeTest is TestBase {
         });
     }
 
-    function test_setRoutes_reverts() public prank(admin) {
+    function test_setRoutes_reverts() public prank(configurer) {
         uint64[] memory chainIds = new uint64[](1);
         chainIds[0] = DEST_CHAIN_ID + 1;
         IBridge.Route[] memory routes;
@@ -119,7 +140,7 @@ contract GeneralBridgeTest is TestBase {
         bridgeWithLockbox.configureRoutes(chainIds, routes);
     }
 
-    function test_setRoutes_succeeds() public prank(admin) {
+    function test_setRoutes_succeeds() public prank(configurer) {
         uint64[] memory chainIds = new uint64[](1);
         chainIds[0] = DEST_CHAIN_ID + 1;
         IBridge.Route[] memory routes = new IBridge.Route[](1);
@@ -133,7 +154,10 @@ contract GeneralBridgeTest is TestBase {
 
         uint64[] memory chainIds = new uint64[](1);
         chainIds[0] = DEST_CHAIN_ID + 1;
+        IBridge.Route[] memory routes = new IBridge.Route[](1);
+        routes[0] = IBridge.Route({ bridge: makeAddr("newBridge"), hasLockbox: false });
+
         vm.prank(authorizer);
-        bridgeWithLockbox.authorizeRoutes(chainIds);
+        bridgeWithLockbox.authorizeRoutes(chainIds, routes);
     }
 }