Decent - Tendency's results

Lines of code

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

Vulnerability details

Impact

DcntEth::setRouter function is currently wrongly left unrestricted, thus callable by everyone, as seen:

    function setRouter(address _router) public {
        router = _router;
    }

Here are some of the effects of this:

• A malicious user could cause financial losses for DecentEthRouter liquidity providers, by updating DcntEth router address to another address(e.g. the attacker-controlled address), thus, reverting all calls from DecentEthRouter contract, since the router now points to the user's set address. Hence, blocking all user's calls to DecentEthRouter::removeLiquidityWeth and removeLiquidityEth functions.

    address public router;

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

• Assuming the attacker is bob, bob can call DcntEth::setRouter(bobAddress), to set the router to his address, then make another call to DcntEth::mint(bobAddress, amount) function to mint an arbitrary amount ofDcntEth token to self, since the caller is the router(bob's address), bob will be minted with the input amount. Bob can then go on to redeem the tokens for weth or eth from DecentEthRouter::redeemWeth or redeemEth function.

Note that the amount Bob will be redeeming, will be less than or equal to the available liquidity(i.e. the contract weth balance).

• Also assuming the attacker is Bob, Bob can set the router to a controlled address, and then call DcntEth::burn function to burn any user's token.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Update DcntEth::setRouter function to:

    function setRouter(address _router) public onlyOwner {
        router = _router;
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

If a user wishes to bridge assets from one chain to another UTB::bridgeAndExecute function will be interacted with.

Assuming DecentBridgeAdapter will be used, on call to this function, the following further calls are made: UTB::bridgeAndExecute --> UTB::callBridge --> DecentBridgeAdapter::bridge --> DecentEthRouter::bridgeWithPayload At DecentEthRouter::bridgeWithPayload function, the payload is computed via _getCallParams private function as:

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

The set msg.sender from the payload here, is intended to be used as the refund address in the destination chain if call execution fails and when executing a refund for _executeWeth calls, as we will see shortly. There are two issues with the current implementation:

• When the call originates from UTB::bridgeAndExecute function, the caller here(msg.sender) will be the DecentBridgeAdapter contract and not the user. With this, on failure in the destination chain execution, the user's assets are instead sent to the adapter's address.

• The owner of an address in chain A, might not be the owner of the address in chain B. Assuming the caller interacted with DecentEthRouter::bridgeWithPayload function directly the set msg.sender in the payload will indeed be the caller, but if the caller's address isn't controlled by the caller in the destination chain, the user will lose his assets on failure.

Going further on the execution:

DecentEthRouter::_bridgeWithPayload, further calls dcntEth.sendAndCall, sendAndCall will further go on to send the payload over to LayerZero endpoint. On the destination chain, dcntEth::lzReceive function will be called by LayerZero endpoint, which will go on to call the destination chain DecentEthRouter::onOFTReceived function down its execution, this function also further calls DecentBridgeExecutor::execute function. DecentBridgeExecutor::execute function handles the execution of the passed-in payload, which on revert(i.e bridging failed), refunds assets to the from address(remember this will be the source bridge adapter address when the call originates from UTB).

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

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

As stated in the contest Readme:

Any edge cases should be properly handled such that the user is issued a refund on the destination chain.

The expected implementation is that users be refunded their assets in the destination chain, on failure.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Update DecentEthRouter::bridgeWithPayload and bridge function to allow users to pass in a destination bridge refund address, then update _getCallParams to take in this address. Instead of passing in the msg.sender, use the input refund address:

        if (msgType == MT_ETH_TRANSFER) {
            payload = abi.encode(msgType, refundAddress, _toAddress, deliverEth); //<--@
        } else {
            payload = abi.encode(
                msgType,
                refundAddress, //<--@
                _toAddress,
                deliverEth,
                additionalPayload
            );
        }

Now, in DecentBridgeAdapter::bridge function, pass the already passed in refund address to the DecentEthRouter::bridgeWithPayload call params

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

When a user wishes to perform cross-chain txns UTB::bridgeAndExecute function will be interacted with. This function performs cross-chain calls, leveraging layerZero messaging. Down the execution of this function, the private function callBridge is invoked, which then further makes an external call to the bridge adapter based on the based in bridgeId instruction. Assuming the passed in bridgeId instruction is 0, the DecentBridgeAdapter::bridge function will be called, which will further call DecentEthRouter::bridgeWithPayload function. The issue comes with how _bridgeWithPayload sets the layerZero call params refund address:

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

The refund address is currently being set to the bridge adapter(the message sender), whenever the call path originates from UTB::bridgeAndExecute. note: the refund address is used by layer zero to refund the excess sent-in native fee:

        // assert the user has attached enough native token for this address
        require(totalNativeFee <= msg.value, "LayerZero: not enough native for fees");
        // refund if they send too much
        uint amount = msg.value.sub(totalNativeFee);
        if (amount > 0) {
            (bool success, ) = _refundAddress.call{value: amount}("");
            require(success, "LayerZero: failed to refund");
        }

With the current implementation, the bridge adapters will be refunded the user 's sent in excess native fee, this also goes contrary to the sponsor's note in the contest 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.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add an extra address param to DecentEthRouter::bridgeWithPayload function, to take in a refund address, rather than use the msg.sender. Also, refactor the preceding function calls from UTB::bridgeAndExecute to allow passing in a layerZero refund address.

Assessed type

ETH-Transfer