Description

The MainEscrow and BridgeEscrow contracts have certain centralization risks, which could result in a single point of failure or grant excessive control over the tokens stored in the escrow to a single entity.

For example withdrawEscrow allows owner to withdraw all the tokens from the escrow

function withdrawEscrow(address[] calldata tokens) external onlyOwner onlyBroke {
        uint256 length = tokens.length;

        for (uint256 i; i < length; ++i) {
            IERC20 token = IERC20(tokens[i]);
            uint256 balance = token.balanceOf(address(this));
            if (balance != 0) {
                token.safeTransfer(msg.sender, token.balanceOf(address(this)));
            }
        }
    }

and the bridge functions in BridgeEscrow allow owner to transfer tokens to any address on the L2.

function bridgeTokenArb(address token, address arbEscrow, uint256 amount, uint256 maxGas, uint256 gasPrice)
       external
       onlyOwner
       onlyBroke
   {
       IERC20(token).safeIncreaseAllowance(address(bridgeRouter), amount);
       bridgeRouter.outboundTransferCustomRefund(token, msg.sender, arbEscrow, amount, maxGas, gasPrice, bytes(""));
   }

   function bridgeTokenConnext(address token, address arbEscrow, uint256 amount, uint256 slippage, uint256 relayerFee)
       external
       onlyOwner
       onlyBroke
   {
       IERC20(token).safeIncreaseAllowance(address(renzoLockbox), amount);

       IXERC20 xToken = renzoLockbox.XERC20();
       renzoLockbox.deposit(amount);

       IERC20(address(xToken)).safeIncreaseAllowance(address(connext), amount);
       connext.xcall{value: relayerFee}(
           1634886255, // _destination: Domain ID of the destination chain
           arbEscrow, // _to: address receiving the funds on the destination
           address(xToken), // _asset: address of the token contract
           msg.sender, // _delegate: address that can revert or forceLocal on destination
           amount, // _amount: amount of tokens to transfer
           slippage, // _slippage: the maximum amount of slippage the user will accept in BPS (e.g. 30 = 0.3%)
           bytes("") // _callData: empty bytes because we're only sending funds
       );
   }

Impact

If the owner's keys are compromised, the impact on the protocol could be significant as the contract allows the owner to directly transfer user's assets to another address.

Recommendations

To address this risk, and increase user confidence and security, we recommend implementing measures that remove reliance on a single point of failure. Here are a few recommendations to do that:

• Utilize a multi-signature address wallet with multiple signers. This approach would prevent an attacker from causing economic harm if a private key were compromised.

• Secure critical functions behind a timelock. This would provide users with sufficient time to withdraw funds from the protocol if any malicious executions are scheduled.

• Provide documentation on privileged functions so that users are aware of the potential risks when depositing their tokens into the protocol.

Remediation

Bracket Labs Group SA provided the following response:

Because brktETH will be actively developed during the escrow phase, a certain level of centralization is required as assets need to be exchanged for brktETH once the escrow breaks. However, privileged actions can only be performed by the owner, which will be a big multisig to increase decentralization. Moreover, assets cannot be pulled before the escrow breaks, and users are free to deposit and withdraw before then. Once the escrow breaks, the assets will be exchanged for brktETH and users will be able to claim brktETH in the escrow which will hold an equal value to that of their assets at the moment of break.