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

2 stars 2 forks source link

`bridgeRouterFeeBps` is also getting transfered in `xRenzoDeposit::sweep()` #331

Closed c4-bot-8 closed 6 months ago

c4-bot-8 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L385 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L421

Vulnerability details

While depositing token in xRenzoDeposit through deposit() or depositETH(), a bridgeRouterFeeBps is taken in collateralToken(nextWETH) and stores in the contract itself

     // Subtract the bridge router fee
      @>  if (bridgeRouterFeeBps > 0) {
            uint256 fee = (amountNextWETH * bridgeRouterFeeBps) / 10_000;
            amountNextWETH -= fee;
        }

xRenzoDeposit::sweep() deposits the collateralToken(nextWETH) to the bridge(which further deposits into Renzo), but the problem is it deposits all the collateralToken present in the contract to the bridge, including fee collected from bridgeRouterFeeBps

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

In the above code, we can see collateralToken.balanceOf(address(this)) ie complete balance is transfered to connext.xcall

Impact

Fee accumulated from bridgeRouterFeeBps is lost as it gets deposited to Renzo thorough xRenzoBridge::xReceive()(sweep() calls xRenzoBridge::xReceive())

Proof of Concept

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L421C8-L442C11 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L385C7-L388C10

Tools Used

Manual Review

Recommended Mitigation Steps

Subtract bridgeRouterFeeBps from collateralToken balance before depositing into Renzo

Assessed type

Token-Transfer

DadeKuma commented 6 months ago

@howlbot accept