c4-bot-8 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L434-L442 https://github.com/connext/monorepo/blob/8338d6506c609f9383d81133c3cb40cfb9e44392/packages/deployments/contracts/contracts/core/connext/facets/BridgeFacet.sol#L303

Vulnerability details

Impact

Malicious user can have undue control over funds that are bridged to L1, leading to loss of funds.

Proof of Concept

it is common knowledge that when bridging funds across chains, setting the recipient or even _delegate to msg.sender can lead to loss of funds if the msg.sender account is an abstraction account or simply a contract, this is because the address on the destination chain will differ form the address on the originating chain where the funds were sent from.

When xRenzoDeposit.sweep(...) is called to bridge nextWETH to L1, an internal call is made to connext.xcall(...), however the problem is that the _delegate parameter of the xcall(...) function is hardcoded to msg.sender

    function sweep() public payable nonReentrant {
        ...

        // @audit funds could be lost during sweeping or when bridging to L1 because delegate will be different on the destination chain
        connext.xcall{ value: msg.value }(
            bridgeDestinationDomain,
            bridgeTargetAddress,
            address(collateralToken),
            msg.sender, 
            balance,
            0, // Asset is already nextWETH, so no slippage will be incurred
            bridgeCallData
        );

        // send collected bridge fee to sweeper
        _recoverBridgeFee();

        // Emit the event
        emit BridgeSwept(bridgeDestinationDomain, bridgeTargetAddress, msg.sender, balance);
    }

According to connext, the _delegate parameter which is an address on destination domain that has rights to update slippage tolerance, retry transactions, or revert back to origin in the event that a transaction fails at the destination is set to msg.sender.

The problem is, if the msg.sender that is an allowedBridgeSweepers[msg.sender] is a contract address, the owner of the msg.sender contract on the destination domain or chain may differ from the originating chain, giving a different (perhaps malicious) user undue control over the the funds if the transaction fails on the destination chain.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using a trusted and verified L1 address as the _delegate instead of hardcoding the _delegate to msg.sender.

Assessed type

Access Control

raymondfam commented 3 months ago

Too generic of the description with inadequate proof that account abstraction, which is a common feature of many bridges, isn't available on Connext.

https://docs.connext.network/usecases/chain-abstraction