code-423n4 / 2024-04-renzo-validation

2 stars 2 forks source link

Old Sweepers Could Steal Bridge Fees Through Front-Running Before Being Disallowed in ``setAllowedBridgeSweeper()`` #676

Closed c4-bot-5 closed 6 months ago

c4-bot-5 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L466-L470 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L414-L449 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L396-L406

Vulnerability details

Impact

The most direct impact is the theft of collected bridge fees. If a sweeper is no longer supposed to have access but can still initiate a sweep due to front-running the permission revocation, they can transfer potentially significant amounts of bridge fees to themselves. This results in a direct financial loss to the protocol and its users.

Proof of Concept


 function setAllowedBridgeSweeper(address _sweeper, bool _allowed) external onlyOwner {
        allowedBridgeSweepers[_sweeper] = _allowed;

        emit BridgeSweeperAddressUpdated(_sweeper, _allowed);
    }

function sweep() public payable nonReentrant {
        // Verify the caller is whitelisted
        if (!allowedBridgeSweepers[msg.sender]) {
            revert UnauthorizedBridgeSweeper();
        }

        // Get the balance of nextWETH in the contract
        uint256 balance = collateralToken.balanceOf(address(this));

        // If there is no balance, return
        if (balance == 0) {
            revert InvalidZeroOutput();
        }

        // Approve it to the connext contract
        collateralToken.safeApprove(address(connext), balance);

        // Need to send some calldata so it triggers xReceive on the target
        bytes memory bridgeCallData = abi.encode(balance);

        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);
    }

 function _recoverBridgeFee() internal {
        uint256 feeCollected = bridgeFeeCollected;
        bridgeFeeCollected = 0;
        // transfer collected fee to bridgeSweeper
        uint256 balanceBefore = address(this).balance;
        IWeth(address(depositToken)).withdraw(feeCollected);
        feeCollected = address(this).balance - balanceBefore;
        (bool success, ) = payable(msg.sender).call{ value: feeCollected }("");
        if (!success) revert TransferFailed();
        emit SweeperBridgeFeeCollected(msg.sender, feeCollected);
    }

POC


Transaction 1 (Pending): setAllowedBridgeSweeper(sweeperA, false)
Transaction 2 (Front-Running): sweeperA calls sweep()

Step 1: Initial Setup

Step 2: Change in Permissions

Step 3: Front-Running Opportunity

Step 5: Confirmation of Transactions

Tools Used

Manual Audit

Recommended Mitigation Steps

Introduce a timelock mechanism that delays the enforcement of permission changes. For instance, when setAllowedBridgeSweeper is called, the change could be stored in a pending state and only applied after a certain cooling-off period (e.g., 24 hours).

Assessed type

MEV

DadeKuma commented 6 months ago

@howlbot accept