Decent - haxatron's results

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DcntEth.sol#L20

Vulnerability details

Impact

There is a missing access control check in setRouter in DcntEth.sol.

DcntEth.sol#L5C1-L39C2

contract DcntEth is OFTV2 {
    address public router;

    modifier onlyRouter() {
        require(msg.sender == router);
        _;
    }

    constructor(
        address _layerZeroEndpoint
    ) OFTV2("Decent Eth", "DcntEth", 18, _layerZeroEndpoint) {}

    /**
     * @param _router the decentEthRouter associated with this eth
     */
=>  function setRouter(address _router) public {
        router = _router;
    }

    function mint(address _to, uint256 _amount) public onlyRouter {
        _mint(_to, _amount);
    }

    function burn(address _from, uint256 _amount) public onlyRouter {
        _burn(_from, _amount);
    }

    function mintByOwner(address _to, uint256 _amount) public onlyOwner {
        _mint(_to, _amount);
    }

    function burnByOwner(address _from, uint256 _amount) public onlyOwner {
        _burn(_from, _amount);
    }
}

Simply put, anyone can call setRouter and set themselves as router, allowing them to bypass the onlyRouter check and mint DcntEth tokens to anyone or burn DcntEth tokens from anyone.

The dcntEth can be redeeemed on the real router to drain its WETH reserves.

DecentEthRouter.sol#L294-L299

    function redeemWeth(
        uint256 amount
    ) public onlyIfWeHaveEnoughReserves(amount) {
        dcntEth.transferFrom(msg.sender, address(this), amount);
        weth.transfer(msg.sender, amount);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add onlyOwner modifier to setRouter.

Assessed type

Access Control

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L280

Vulnerability details

Impact

When the transaction on the bridge executor on the destination bridge fails, it will refund the tokens to the address specified by _from address. This _from address is the source chain address of the source chain's bridge adapter.

Therefore when the call from bridge executor fails (it can be due to slippage checks failing during swapping in UTB in the destination chain), then the funds are sent to the _from address in the destination chain, which is not controlled by any user.

The following shows that the executor will refund tokens / ETH to the from address DecentBridgeExecutor.sol#L24C1-L65C6

    function _executeEth(
        address from,
        address target,
        uint256 amount,
        bytes memory callPayload
    ) private {
        weth.withdraw(amount);
        (bool success, ) = target.call{value: amount}(callPayload);
        if (!success) {
=>           payable(from).transfer(amount);
        }
    }
    ... // omitted executeWeth as it is largely the same as _executeEth
    function execute(
        address from,
        address target,
        bool deliverEth,
        uint256 amount,
        bytes memory callPayload
    ) public onlyOwner {
        weth.transferFrom(msg.sender, address(this), amount);

        if (!gasCurrencyIsEth || !deliverEth) {
=>          _executeWeth(from, target, amount, callPayload);
        } else {
=>          _executeEth(from, target, amount, callPayload);
        }
    }

The from address, however, is the msg.sender which calls bridgeWithPayload on the source chain

Observe that on the destination chain, the from/_from address is taken from the 2nd slot of the payload.

DecentEthRouter.sol#L237C1-L282C6

    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)
            );
        }
        ...
        } else {
            weth.approve(address(executor), _amount);
            executor.execute(_from, _to, deliverEth, _amount, callPayload);
        }
    }

The source chain encodes the msg.sender of the payload into the 2nd slot. (Note: the function below is called by _bridgeWithPayload which is called by bridgeWithPayload) DecentEthRouter.sol#L80C1-L111C1

    function _getCallParams(
        uint8 msgType,
        address _toAddress,
        uint16 _dstChainId,
        uint64 _dstGasForCall,
        bool deliverEth,
        bytes memory additionalPayload
    )
        ...
        if (msgType == MT_ETH_TRANSFER) {
=>          payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
        } else {
            payload = abi.encode(
                msgType,
=>              msg.sender,
                _toAddress,
                deliverEth,
                additionalPayload
            );
        }
    }

However, msg.sender that calls bridgeWithPayload is actually the source chain's bridge adapter: DecentBridgeAdapter.sol#L81C1-L125C6

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

Hence, when the executor attempts to refund the cross-chain transaction on a failed call (which can be due to a failed swap in UTB), then the tokens will be sent to the _from address which is not controlled by either the user or Decent (because contracts may not necessarily use the same address on different chains), leading to a lost transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

Pass a destination chain refund address into the payload sent cross-chain and replace the from address used in the executor with it.

Assessed type

Other

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L267

Vulnerability details

Impact

When the DecentEthRouter receives the dcntEth OFT token from a cross-chain transaction, if the WETH balance of the destination router is less than amount of dcntEth received (this could be due to the router receiving more cross-chain transactions than than sending cross-chain transactions which depletes its WETH reserves), then the dcntEth will get transferred to the address specified by _to.

DecentEthRouter.sol#L266-L281

    function onOFTReceived(
        uint16 _srcChainId,
        bytes calldata,
        uint64,
        bytes32,
        uint _amount,
        bytes memory _payload
    ) external override onlyLzApp {
        ...
        if (weth.balanceOf(address(this)) < _amount) {
=>          dcntEth.transfer(_to, _amount);
            return;
        }

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

This dcntEth is sent to the user so that they can either redeem the WETH / ETH from the router once the WETH balance is refilled or send it back to the source chain to redeem back the WETH.

The problem is that if the msgType != MT_ETH_TRANSFER, then the _to address is not the user, it is instead the target meant to be called by the destination chain's bridge executor (if the source chain uses a decent bridge adapter, the target is always the destination chain's bridge adapter which does not have a way to withdraw the dcntEth)

The following snippet shows what occurs in the bridge executor (_executeEth omitted as it does largely the same thing as _executeWeth):

DecentBridgeExecutor.sol#L24-L82

    function _executeWeth(
        address from,
        address target,
        uint256 amount,
        bytes memory callPayload
    ) private {
        uint256 balanceBefore = weth.balanceOf(address(this));
        weth.approve(target, amount);

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

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

        uint256 remainingAfterCall = amount -
            (balanceBefore - weth.balanceOf(address(this)));

        // refund the sender with excess WETH
        weth.transfer(from, remainingAfterCall);
    }
    ...
    function execute(
        address from,
        address target,
        bool deliverEth,
        uint256 amount,
        bytes memory callPayload
    ) public onlyOwner {
        weth.transferFrom(msg.sender, address(this), amount);

        if (!gasCurrencyIsEth || !deliverEth) {
            _executeWeth(from, target, amount, callPayload);
        } else {
            _executeEth(from, target, amount, callPayload);
        }
    }

Therefore, once the dcntEth is transferred to the execution target (which is almost always the destination chain bridge adapter, see Appendix for the code walkthrough). The user cannot do anything to retrieve the dcntEth out of the execution target, so the cross-chain transaction is lost.

Tools Used

Manual Review

Recommended Mitigation Steps

Pass a destination chain refund address into the payload sent cross-chain and replace the _to address used in DecentEthRouter.sol#L267:

        if (weth.balanceOf(address(this)) < _amount) {
        // REPLACE '_to' with the destination chain refund address
=>          dcntEth.transfer(_to, _amount);
            return;
        }

Appendix (May ignore)

To see why the target is always the destination bridge adapter if the source chain is a decent bridge adapter:

UTB.sol will first call the bridge function in the adapter with the destination bridge adapter address as the 2nd argument.

DecentBridgeAdapter.sol#L117C1-L124C11

    function bridge(
        ...
        router.bridgeWithPayload{value: msg.value}(
            lzIdLookup[dstChainId],
            destinationBridgeAdapter[dstChainId],
            swapParams.amountIn,
            false,
            dstGas,
            bridgePayload
        );
    }

which calls the below function in the router, where the _toAddress is the 2nd argument and therefore is the destination bridge adapter address

DecentEthRouter.sol#L196C1-L204C23

    /// @inheritdoc IDecentEthRouter
    function bridgeWithPayload(
        uint16 _dstChainId,
        address _toAddress,
        uint _amount,
        bool deliverEth,
        uint64 _dstGasForCall,
        bytes memory additionalPayload
    ) public payable {
           return
            _bridgeWithPayload(
                MT_ETH_TRANSFER_WITH_PAYLOAD,
                _dstChainId,
                _toAddress,
                _amount,
                _dstGasForCall,
                additionalPayload,
                deliverEth
            );
           ...

which calls _bridgeWithPayload which calls _getCallParams to encode the payload to send to the destination chain:

DecentEthRouter.sol#L148C1-L168C15

    function _bridgeWithPayload(
        uint8 msgType,
        uint16 _dstChainId,
        address _toAddress,
        uint _amount,
        uint64 _dstGasForCall,
        bytes memory additionalPayload,
        bool deliverEth
    ) internal {
        (
            bytes32 destinationBridge,
            bytes memory adapterParams,
            bytes memory payload
        ) = _getCallParams(
                msgType,
                _toAddress,
                _dstChainId,
                _dstGasForCall,
                deliverEth,
                additionalPayload
            );
            ...

The _toAddress parameter is always the 3rd parameter in the payload sent.

DecentEthRouter.sol#L101-L110

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

Which matches _to variable in onOFTReceived

DecentEthRouter.sol#L236

    /// @inheritdoc IOFTReceiverV2
    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)
            );
        }
        ...

Assessed type

Other

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L197-L234

Vulnerability details

Impact

In https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L197-L234, anyone can call bridge and bridgeWithPayload

    function bridgeWithPayload(
        uint16 _dstChainId,
        address _toAddress,
        uint _amount,
        bool deliverEth,
        uint64 _dstGasForCall,
        bytes memory additionalPayload
    ) public payable {
        return
            _bridgeWithPayload(
                MT_ETH_TRANSFER_WITH_PAYLOAD,
                _dstChainId,
                _toAddress,
                _amount,
                _dstGasForCall,
                additionalPayload,
                deliverEth
            );
    }

    /// @inheritdoc IDecentEthRouter
    function bridge(
        uint16 _dstChainId,
        address _toAddress,
        uint _amount,
        uint64 _dstGasForCall,
        bool deliverEth // if false, delivers WETH
    ) public payable {
        _bridgeWithPayload(
            MT_ETH_TRANSFER,
            _dstChainId,
            _toAddress,
            _amount,
            _dstGasForCall,
            bytes(""),
            deliverEth
        );
    }

These functions are meant to be called by the bridge adapter only which is called by UTB which takes a fee, but they can called by anyone as they have no modifiers set.

Therefore, a user can send a cross-chain transaction without paying fees to Decent by directly calling bridge and bridgeWithPayload. A secondary impact is that this will also deplete the ETH / WETH reserves of the router in the destination chain.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add access control checks to these functions (only bridge adapter should call these functions)

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTB.sol#L311

Vulnerability details

Impact

Whenever a user wants to call swapAndExecute (which performs a swap and executes a transaction for a user in the same chain), they have to pay a fee via retrieveAndCollectFees modifier.

UTB.sol#L311C1-L319C6

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

However there is another function that does essentially the same thing without the retrieveAndCollectFees modifier.

UTB.sol#L311C1-L319C6

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

Essentially, this means a user can bypass fees in swapAndExecute by using the function receiveFromBridge.

Note that though this function is not payable, _swapAndExecute also accepts ERC-20 tokens, meaning that one can abuse receiveFromBridge to swap ERC-20 tokens and execute transactions without a fee.

Tools Used

Manual Review

Recommended Mitigation Steps

The receiveFromBridge function needs to check if the msg.sender is actually a bridge adapter by using a new storage mapping that stores a boolean whether a given address is a valid bridge adapter.

Assessed type

Access Control

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L171

Vulnerability details

Impact

When setting up the LzCallParams for the cross-chain transaction the refund address is set to the msg.sender of the transaction.

DecentEthRouter.sol#L170C1-L174C12

        ICommonOFT.LzCallParams memory callParams = ICommonOFT.LzCallParams({
=>          refundAddress: payable(msg.sender),
            zroPaymentAddress: address(0x0),
            adapterParams: adapterParams
        });

The refundAddress in LzCallParams is the address to pay if the fee paid for the cross-chain LayerZero transaction is overpaid (this includes gas fees for the transaction). See here.

However, the msg.sender which calls bridgeWithPayload is the source chain bridge adapter

DecentBridgeAdapter.sol#L81C2-L125C6

    function bridge(
        ...
    ) public payable onlyUtb returns (bytes memory bridgePayload) {
        ...
=>      router.bridgeWithPayload{value: msg.value}(
            lzIdLookup[dstChainId],
            destinationBridgeAdapter[dstChainId],
            swapParams.amountIn,
            false,
            dstGas,
            bridgePayload
        );
    }

Therefore, if the gasValue passed into dcntEth.sendAndCall is greater than the gas cost of of the cross-chain transaction, the excess value is refunded and stuck in the source chain bridge adapter instead of the user who initiated a cross chain transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users to pass in a source chain refund address which will get refunded the excess gas fee.

Assessed type

Other