Decent - deth's results

Lines of code

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

Vulnerability details

Impact

DcntEth implements a special variable, router, which allows that specific address to freely call mint and burn.

Currently the function has no access control, so anyone can set themselves as the router and freely mint and burn DcntEth tokens.

Proof of Concept

1. The protocol deploys DcntEth and call setRouter, and set a trusted address to be the router.
2. Alice sees that the function setRouter has no access control.
3. Alice calls setRouter and sets her address as the router.
4. Alice can now freely mint, and burn anyone's DcntEth tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the onlyOwner modifier to setRouter

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The DecentBridgeExecutor is used to handle calls to the DecenBridgeAdapter on the destination chain, when a user wants to bridge from Chain A to Chain B.

Looking at the two different types of execute functions:

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

You'll notice that if any of the two calls fail, the amount will be transferred back to the from address.

The comment inside _executeWeth even states that the tokens will be refunded to the sender and even the nat specs of the functions state.

@param from caller of the function

The problem is that the caller of the function ins't the from address.

execute is being called inside DecentEthRouter.

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

        emit ReceivedDecentEth(
            msgType,
            _srcChainId,
            _from,
            _to,
            _amount,
            callPayload
        );

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

You can see that the _from address which is supposed to be the caller of the function, is actually an address that is decoded from _payload

(uint8 msgType, address _from, address _to, bool deliverEth) = abi
            .decode(_payload, (uint8, address, address, bool));

This _payload is the payload that is encoded inside _getCallParams inside DecentEthRouter on the source chain.

function _getCallParams(
        uint8 msgType,
        address _toAddress,
        uint16 _dstChainId,
        uint64 _dstGasForCall,
        bool deliverEth,
        bytes memory additionalPayload
    )
        private
        view
        returns (
            bytes32 destBridge,
            bytes memory adapterParams,
            bytes memory payload
        )
    {
        uint256 GAS_FOR_RELAY = 100000;
        uint256 gasAmount = GAS_FOR_RELAY + _dstGasForCall;
        adapterParams = abi.encodePacked(PT_SEND_AND_CALL, gasAmount);
        destBridge = bytes32(abi.encode(destinationBridges[_dstChainId]));
        if (msgType == MT_ETH_TRANSFER) {
->          payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
        } else {
->          payload = abi.encode(
                msgType,
                msg.sender,
                _toAddress,
                deliverEth,
                additionalPayload
            );
        }
    }

You can see that the payload is encoded here and msg.sender is used as the _from address.

This is problematic, as msg.sender is the DecentBridgeAdapter on the source chain.

Proof of Concept

If the target.call inside either _executeWeth or _executeEth fails, the funds will be sent to the address of the DecentBridgeAdapter on the source chain.

1. The address of from is the DecentBridgeAdapter, and it is the same on both chains, in which case the funds will get stuck inside the DecentBridgeAdapter as the contract has no way of withdrawing either ETH or WETH.
2. The address of from isn't any protocol address, in which case the funds will be sent to a random address that isn't protocol controlled, and will be lost. 2.1. If _executeEth attempts to refund the ETH to a from address that isn't protocol controlled and can't accept ETH, any tx's that attempt to refund the tokens will fail, causing a major DoS on the system.

Tools Used

Manual Review

Recommended Mitigation Steps

Refund the tokens to msg.sender instead of the from address, as msg.sender is the DecentEthRouter which can handle both ETH and WETH.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

onOFTReceived is used to handle the call from DcntEth on the destination chain, when a user attempts to bridgeAndExecute from one chain to another.

The flow is as follows: On Chain A: UTB.bridgeAndExecute() -> DecentBridgeAdapter -> DecentEthRouter -> DcnEth -> LayerZero

On Chain B: DcnEth -> DecentEthRouter -> DecentBridgeExecutor -> DecentBridgeAdapter -> UTB -> UTBExecutor -> any arbitrary target address with arbitrary payload

We'll focus on the part where DcntEth calls DecentEthRouter on Chain B.

function callOnOFTReceived(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        uint64 _nonce,
        bytes32 _from,
        address _to,
        uint _amount,
        bytes calldata _payload,
        uint _gasForCall
    ) public virtual {
        require(_msgSender() == address(this), "OFTCore: caller must be OFTCore");

        // send
        _amount = _transferFrom(address(this), _to, _amount);
        emit ReceiveFromChain(_srcChainId, _to, _amount);

        // call
        IOFTReceiverV2(_to).onOFTReceived{gas: _gasForCall}(_srcChainId, _srcAddress, _nonce, _from, _amount, _payload);
    }

This is the function that will call onOFTReceived inside DecentEthRouter.

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

        emit ReceivedDecentEth(
            msgType,
            _srcChainId,
            _from,
            _to,
            _amount,
            callPayload
        );

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

You'll notice this check:

if (weth.balanceOf(address(this)) < _amount) {
            dcntEth.transfer(_to, _amount);
            return;
        }

Basically this will check the WETH balance of the DecentEthRouter and if it's smaller than amount, it will send DcntEth tokens to the _to address instead and finish the tx, by returning.

The _to address is inside _payload. This payload is constructed on Chain A inside _getCallParams.

 function _getCallParams(
        uint8 msgType,
        address _toAddress,
        uint16 _dstChainId,
        uint64 _dstGasForCall,
        bool deliverEth,
        bytes memory additionalPayload
    )
        private
        view
        returns (
            bytes32 destBridge,
            bytes memory adapterParams,
            bytes memory payload
        )
    {
        uint256 GAS_FOR_RELAY = 100000;
        uint256 gasAmount = GAS_FOR_RELAY + _dstGasForCall;
        adapterParams = abi.encodePacked(PT_SEND_AND_CALL, gasAmount);
        destBridge = bytes32(abi.encode(destinationBridges[_dstChainId]));
        if (msgType == MT_ETH_TRANSFER) {
            payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
        } else {
            payload = abi.encode(
                msgType,
                msg.sender,
                _toAddress,
                deliverEth,
                additionalPayload 
            );
        }
    }

The _toAddress is sent when router.bridgeWithPayload is called inside DecentBridgeAdapter on Chain A.

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

The _toAddress is destinationBridgeAdapter based on dstChainId. Basically it's the address of the DestinationBridgeAdapter on Chain B.

Going back to this check inside onOFTReceived.

if (weth.balanceOf(address(this)) < _amount) {
            dcntEth.transfer(_to, _amount);
            return;
        }

If the if statement is true, then DcntEth will be sent to the DecentBridgeAdapter.

This is can be forced by any user on Chain B, that holds a reasonable amount of DcntEth tokens.

The user will call removeLiquidityWeth, which will burn his DcntEth and DecentEthRouter will transfer WETH to him in return. If a user burns enough DcntEth and gets WETH in return, he will force the above if statement.

The problems here can be 2 different ones, but both equate in the same problem. Note: Both of the below are assumptions, as DcnEth has a minting function, I am unsure if the protocol decides to straight up mint tokens to it's contracts or not, thus I am going to explain both scenarios.

Scenario A, DcntETH == WETH In this scenario the total amount of DcntEth is equal to the amount of WETH inside DecentEthRouter, thus if the contract has 10 WETH inside it, then it also has 10DcntETH, so if the user above if statement is entered, the tx will revert, since DcntETH == WETH in the contract, if (weth.balanceOf(address(this)) < _amount) is true, then dcntEth.balanceOf(address(this)) < _amount) is also true.

Scenario B, DcntETH > WETH The amount of DcntEth will be sent to the DecentBridgeAdapter and since the contract doesn't implement a function in which it transfers or interacts with DcntEth tokens in any way, the tokens will get stuck in the contract forever, permanently losing the funds of the user that started the bridging tx in the first place.

After the DcntEth tokens get transferred, the code immediately finishes. The user specified callPayoad will thus never be executed on his specified target address.

Proof of Concept

1. Alice wants to swap tokens from Chain A to Chain B.
2. The tx finishes on Chain A and gets put in the mempool of Chain B.
3. DecentEthRouter has 10 WETH and Alice's _amount is 5WETH
4. Bob has 6DcntETH tokens on Chain B. He wants to force a loss of funds on Alice and stop her from calling her specified target address, front runs the tx on Chain B, calling removeLiquidityWeth, burning his 6DcntETH tokens and getting 6 WETH in return.
5. DecentEthRouter now only has 4WETH. The tx on Chain B from LZ, gets executed and onOFTReceived inside DecentEthRouter is hit. 6.1 The tx straight up reverts, as DcntETH == WETH, so now DecentEthRouter has only 4DcntETH and it's attempting to transfer 5DcntETH. 6.2 The DcntETH will be sent to the DecentBridgeAdapter, permanently freezing the funds inside it. The tx will return on the next line as well, meaning Alice's specified target and callPayload won't get executed as well.

The attack will be easier to pull off on chains, where DecentEthRouter has smaller liquidity.

Tools Used

Manual Review

Recommended Mitigation Steps

The tokens shouldn't be transferred to the DecentBridgeAdapter, but to another address. The refund address that the user specifies might be a good choice.

If DcntETH is supposed to equal WETH, then tackling the part where the tx straight up reverts will be a challenge. Keeping a reserve of some other token might be some solution.

I am unsure on how to exactly stop the attack from happening. Since the tx's happen on different chains, there is no way of "locking" removeLiquidityWeth, so I leave this to the protocol team to decide.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The current flow of swapping and bridging tokens using the DecentBridgeAdapter looks like so:

bridgeAndExecute inside UTB is called, passing in the bridgeId of the DecentBridgeAdapter.

function bridgeAndExecute(
        BridgeInstructions calldata instructions,
        FeeStructure calldata fees,
        bytes calldata signature
    )
        public
        payable
        retrieveAndCollectFees(fees, abi.encode(instructions, fees), signature)
        returns (bytes memory)
    {
        (
            uint256 amt2Bridge,
            BridgeInstructions memory updatedInstructions
        ) = swapAndModifyPostBridge(instructions);
        return callBridge(amt2Bridge, fees.bridgeFee, updatedInstructions);
    }

This then makes a call to callBridge, which will call bridge on the DecentBridgeAdapter.

function callBridge(
        uint256 amt2Bridge,
        uint bridgeFee,
        BridgeInstructions memory instructions
    ) private returns (bytes memory) {
        bool native = approveAndCheckIfNative(instructions, amt2Bridge);
        return
            IBridgeAdapter(bridgeAdapters[instructions.bridgeId]).bridge{
                value: bridgeFee + (native ? amt2Bridge : 0)
            }(
                amt2Bridge,
                instructions.postBridge,
                instructions.dstChainId,
                instructions.target,
                instructions.paymentOperator,
                instructions.payload,
                instructions.additionalArgs,
                instructions.refund
            );
    }

DecentBridgeAdapter then makes a call to the bridgeWithPayload inside DecentEthRouter.

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) {
        require(
            destinationBridgeAdapter[dstChainId] != address(0),
            string.concat("dst chain address not set ")
        );

        uint64 dstGas = abi.decode(additionalArgs, (uint64));

        bridgePayload = abi.encodeCall(
            this.receiveFromBridge,
            (postBridge, target, paymentOperator, payload, refund)
        );

        SwapParams memory swapParams = abi.decode(
            postBridge.swapPayload,
            (SwapParams)
        );

        if (!gasIsEth) {
            IERC20(bridgeToken).transferFrom(
                msg.sender,
                address(this),
                amt2Bridge
            );
            IERC20(bridgeToken).approve(address(router), amt2Bridge);
        }

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

bridgeWithPayoad calls the internal function _bridgeWithPayload which starts the call to LZ and the bridging process itself.

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

        ICommonOFT.LzCallParams memory callParams = ICommonOFT.LzCallParams({
            refundAddress: payable(msg.sender), //@audit-issue all refunded tokens will be sent to the DecentBridgeAdapter
            zroPaymentAddress: address(0x0),
            adapterParams: adapterParams
        });

        uint gasValue;
        if (gasCurrencyIsEth) {
            weth.deposit{value: _amount}();
            gasValue = msg.value - _amount;
        } else {
            weth.transferFrom(msg.sender, address(this), _amount);
            gasValue = msg.value;
        }

        dcntEth.sendAndCall{value: gasValue}(
            address(this), // from address that has dcntEth (so DecentRouter)
            _dstChainId,
            destinationBridge, // toAddress
            _amount, // amount
            payload, //payload (will have recipients address)
            _dstGasForCall, // dstGasForCall
            callParams // refundAddress, zroPaymentAddress, adapterParams
        );
    }

When we are using LZ, we have to specify LzCallParams. The struct holds several things, but importantly it holds the refundAddress

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

You'll notice that the refundAddress is specified as msg.sender, in this case msg.sender is the DecentBridgeAdapter since that's the address that made the call to DecentEthRouter.

The refundAddress is used for refunding any excess native tokens (in our case) that are sent to LZ in order to pay for the gas. The excess will be refunded on the source chain.

Basically if you send 0.5 ETH to LZ for gas and LZ only needs 0.1ETH, then 0.4ETH will be sent to the refundAddress.

The problem here is, that the DecentBridgeAdapter has no way of retrieving the funds, as it doesn't implement any withdraw functionality whatsoever.

The protocol team even stated in the README.

Fund Accumulation: Other than the UTBFeeCollector, and DcntEth, the contracts are not intended to hold on to any funds or unnecessary approvals. Any native value or erc20 flowing through the protocol should either get delivered or refunded.

This bug clearly violates what the protocol team expects.

Proof of Concept

Example:

1. Alice wants to swap and bridge some tokens from Ethereum to Arbitrum.
2. For simplicity we'll assume that LZ will need 0.1 ETH in order to pay for gas fees.
3. Alice sends 0.5ETH instead.
4. The flow of functions is executed and the extra 0.4 ETH is refunded from LZ to refundAddress, which is set to msg.sender, which is DecentBridgeAdapter.
5. Alice loses here 0.4 ETH forever, as there is no way to withdraw and send the ETH from the DecentBridgeAdapter to Alice.

Tools Used

Manual Review

Recommended Mitigation Steps

The user specifies a refund when calling bridgeAndExecute inside UTB. Use the address that the user specifies instead of msg.sender.

Assessed type

ETH-Transfer