Decent - Kow's results

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L24-L65

Vulnerability details

Impact

Users will lose their bridged assets if their cross-chain call with a pre-bridging swap fails at the DecentBridgeExecutor.

Proof of Concept

When the onOFTReceived callback is called on DecentEthRouter by the DecentEth OFT token resulting from a bridging transaction, the payload is decoded to provide the parameters for execution using DecentBridgeExecutor if the message type is not MT_ETH_TRANSER (ie. MT_ETH_TRANSFER_WITH_PAYLOAD). https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L237-L282

    function onOFTReceived(
        uint16 _srcChainId,
        bytes calldata,
        uint64,
        bytes32,
        uint _amount,
        bytes memory _payload
    ) external override onlyLzApp {
        (uint8 msgType, address _from, address _to, bool deliverEth) = abi
            .decode(_payload, (uint8, address, address, bool));

        bytes memory callPayload = "";

        if (msgType == MT_ETH_TRANSFER_WITH_PAYLOAD) {
            (, , , , callPayload) = abi.decode(
                _payload,
                (uint8, address, address, bool, bytes)
            );
        }
        ...
        if (msgType == MT_ETH_TRANSFER) {
            if (!gasCurrencyIsEth || !deliverEth) {
                weth.transfer(_to, _amount);
            } else {
                weth.withdraw(_amount);
                payable(_to).transfer(_amount);
            }
        } else {
            weth.approve(address(executor), _amount);
            executor.execute(_from, _to, deliverEth, _amount, callPayload);
        }

The payload being decoded was generated on the calling chain in _getCallParams as part of _bridgeWithPayload which is exposed to users through bridgeWithPayload. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L100-L109

        if (msgType == MT_ETH_TRANSFER) {
            payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
        } else {
            payload = abi.encode(
                msgType,
                msg.sender,
                _toAddress,
                deliverEth,
                additionalPayload
            );

We can see that the _from address is always msg.sender. If bridgeWithPayload on DecentEthRouter was called on the calling chain by the user, this would be the user's address (though we should note that if the caller was not an EOA or was using some form of account abstraction, the user may not have control over this address on the execution chain). Otherwise, bridgeWithPayload may have been called by a bridge adapter if a swap was performed prior to bridging through the UTB, so _from == msg.sender would've been the bridge adapter. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/DecentBridgeAdapter.sol#L81-L125

    function bridge(
        uint256 amt2Bridge,
        SwapInstructions memory postBridge,
        uint256 dstChainId,
        address target,
        address paymentOperator,
        bytes memory payload,
        bytes calldata additionalArgs,
        address payable refund
    ) public payable onlyUtb returns (bytes memory bridgePayload) {
        ...
        router.bridgeWithPayload{value: msg.value}(
            lzIdLookup[dstChainId],
            destinationBridgeAdapter[dstChainId],
            swapParams.amountIn,
            false,
            dstGas,
            bridgePayload
        );
    }

In the external call to the executor, the bridged WETH or ETH is transferred back to from (== _from supplied in parameters of execute) as a refund in the event the external call on target fails. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L30-L38

        uint256 balanceBefore = weth.balanceOf(address(this));
        weth.approve(target, amount);

        (bool success, ) = target.call(callPayload);

        if (!success) {
            weth.transfer(from, amount);
            return;
        }

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L60-L64

        weth.withdraw(amount);
        (bool success, ) = target.call{value: amount}(callPayload);
        if (!success) {
            payable(from).transfer(amount);
        }

However as explained prior, from could be the address of the bridge adapter on the calling chain. Consequently, the bridged funds are lost (this could also be the case where the original caller does not have control over the calling address on the execution chain e.g. gasless transactions using signed messages).

If any of the tests with swaps prior to bridging (e.g. testEth2Weth in UTBExactOutRoutesTestEth2Matic.t.sol) are run with execution trace output, we can see that the DecentBridgeExecutor calls execute with the address of the bridge adapter from the calling chain. The DecentBridgeExecutor.execute function call parameters for the example specified above is shown below where we can see from is the Optimism bridge adapter.

    │   │   │   │   │   ├─ [358072] polygon_DecentBridgeExecutor::execute(
optimism_DecentBridgeAdapter: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c],
polygon_DecentBridgeAdapter: [0x13aa49bAc059d709dd0a18D6bb63290076a702D7], 
false, 
1030000000000000 [1.03e15],
0xc41a776300000000000000000000000000000000000000000000000000000000000000a00000000000000000000000003381cd18e2fb4db236bf0525938ab6e43db0440f0000000000000000000000003381cd18e2fb4db236bf0525938ab6e43db0440f000000000000000000000000000000000000000000000000000000000000024000000000000000000000000000000000000000000000000000000000000a11ce0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000d6bbde9174b1cdaa358d2cf4d57d1a9f7178fbff00000000000000000000000000000000000000000000000000000000000a11ce0000000000000000000000000000000000000000000000000003a8c7901e600000000000000000000000000000000000000000000000000000038d7ea4c680000000000000000000000000007ceb23fd6bc0add59e62ac25578270cff1b9f6190000000000000000000000007ceb23fd6bc0add59e62ac25578270cff1b9f619000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000024ffb3a1fe0000000000000000000000000000000000000000000000000000000000000b0b00000000000000000000000000000000000000000000000000000000)

Tools Used

Manual Review

Recommended Mitigation Steps

Allow the caller to specify a refund address in this payload constructed in the router.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L311-L319 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L108-L124

Vulnerability details

Impact

Users can avoid paying fees for same chain swap-and-execute actions using the decent UTB resulting in a loss of funds for the protocol.

Proof of Concept

In UTB.sol, swapAndExecute allows users to combine swapping and execution of functions on external contracts. A fee is charged for this through the retrieveAndCollectFees modifier which requires callers to first obtain a signed message for applicable fees generated by the protocol. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L108-L124

    function swapAndExecute(
        SwapAndExecuteInstructions calldata instructions,
        FeeStructure calldata fees,
        bytes calldata signature
    )
        public
        payable
        retrieveAndCollectFees(fees, abi.encode(instructions, fees), signature)
    {
        _swapAndExecute(
            instructions.swapInstructions,
            instructions.target,
            instructions.paymentOperator,
            instructions.payload,
            instructions.refund
        );
    }

There is another function with the exact same functionality (it calls the same internal function) and inputs (albeit split into separate parameters rather than a single struct) called recieveFromBridge which is intended to be called by the bridge adapter. It differs in that it does not have the retrieveAndCollectFees modifier, so no fees are required (since the fee would've already been paid on the calling chain). However, it is unrestricted and can be callable by normal users. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L311-L319

    function receiveFromBridge(
        SwapInstructions memory postBridge,
        address target,
        address paymentOperator,
        bytes memory payload,
        address payable refund
    ) public {
        _swapAndExecute(postBridge, target, paymentOperator, payload, refund);
    }

Consequently, users can bypass fees by calling this function instead. It should also be noted that receiveFromBridge is not payable, so swap-and-execute transactions involving native ETH in the initial swap cannot be performed.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a whitelist that restricts access to receiveFromBridge to bridge adapters.

Assessed type

Access Control