Decent - ether_sky's results

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DcntEth.sol#L20-L22 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DcntEth.sol#L24-L26 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L285-L291

Vulnerability details

Impact

The setRouter function exists in DcntEth. Anyone can set any route because this function lacks a modifier. Consequently, a user can create DcntEth tokens as his wish and redeem ETH for them through DecentEthRouter.

Proof of Concept

1. A user has set the route.

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

2. That user mints DcntEth tokens using his router.

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

3. Subsequently, the user withdraws ETH from DecentEthRouter.

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

And the incorrect route can lead to numerous unexpected problems.

Tools Used

Recommended Mitigation Steps

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L273 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L289 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/DecentBridgeAdapter.sol#L117 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L161 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L103-L109 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L185 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L245 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L63 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L36

Vulnerability details

Impact

When users attempt to swap tokens using this protocol, they also specify a refund who can receive the excess funds. However, in the event of a failed external call, the tokens are transferred to the Bridge Adapter on that chain, and there is no way to recover them.

Proof of Concept

1. A user who wishes to exchange a token on Chain 1 for a token on Chain 2 can invoke the bridgeAndExecute function in UTB. This function calls the callBridge function within the same contract.

function bridgeAndExecute() {
    ...
    return callBridge(amt2Bridge, fees.bridgeFee, updatedInstructions);   // <== here
}

The callBridge function calls the bridge function in the DecentBridgeAdapter contract.

function callBridge() {
    return
        IBridgeAdapter(bridgeAdapters[instructions.bridgeId]).bridge{  // <== here
            value: bridgeFee + (native ? amt2Bridge : 0)
        }(
            amt2Bridge,
            instructions.postBridge,
            instructions.dstChainId,
            instructions.target,
            instructions.paymentOperator,
            instructions.payload,
            instructions.additionalArgs,
            instructions.refund       // <== We insert refund as a parameter to the adapter.
        );
}

2. The bridge function calls the bridgeWithPayload function in the DecentEthRouter contract.

function bridge() {
    router.bridgeWithPayload{value: msg.value}(...);   // <== here
}

3. In the _bridgeWithPayload function, we call the _getCallParams function.

function _bridgeWithPayload() {
    (
        bytes32 destinationBridge,
        bytes memory adapterParams,
        bytes memory payload
    ) = _getCallParams(...);    // <== here
}

In the _getCallParams function, the payload to transfer to Chain 2 will be msgType, msg.sender, _to, deliverEth, additionalPayload. Here, the msg.sender is the Bridge Adapter that calls this function.

function _getCallParams() private view returns (bytes32 destBridge, bytes memory adapterParams, bytes memory payload) {
     if (msgType == MT_ETH_TRANSFER) {
        payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
     } else {
        payload = abi.encode(
            msgType,
            msg.sender,    // <== msg.sender is BridgeAdapter
            _toAddress,
            deliverEth,
            additionalPayload
        );
    }
}

4. Subsequently, we send this payload using LayerZero.

function _bridgeWithPayload() {
    (
        bytes32 destinationBridge,
        bytes memory adapterParams,
        bytes memory payload
    ) = _getCallParams(...);  
    dcntEth.sendAndCall{value: gasValue}(  // <== here
        address(this),
        _dstChainId,
        destinationBridge,
        _amount, 
        payload,   // <== here
        _dstGasForCall,
        callParams
    );
}

5. On Chain 2, the onOFTReceived function in the DecentEthRouter contract will be invoked. Here, the _payload is exactly what the user sent on Chain 1. So, the _from will refer to the Bridge Adapter.

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    // <== _from is BridgeAdapter
       .decode(_payload, (uint8, address, address, bool));

    if (msgType == MT_ETH_TRANSFER) {
    } else {
        weth.approve(address(executor), _amount);
        executor.execute(_from, _to, deliverEth, _amount, callPayload);
    }
}

6. This function will invoke the execute function in the DecentBridgeExecutor contract. If the external call to the target contract fails, we send the tokens to the _from, i.e., the Bridge Adapter.

function _executeEth(
    address from,    // <== from is BridgeAdapter
    address target,
    uint256 amount,
    bytes memory callPayload
) private {
    weth.withdraw(amount);
    (bool success, ) = target.call{value: amount}(callPayload);
    if (!success) {
        payable(from).transfer(amount);   // <== here
    }
}

There may be several cases where the call fails. For instance, the user could set incorrect swap parameters or call another contract not part of this protocol, etc.

function _executeWeth(
    address from,   // <== from is BridgeAdapter
    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);   // <== here
        return;
    }
    uint256 remainingAfterCall = amount -
        (balanceBefore - weth.balanceOf(address(this)));
    weth.transfer(from, remainingAfterCall);   // <== here
}

Tools Used

Recommended Mitigation Steps

Set the refund address instead of the Bridge Adapter.

Assessed type

Error

[L-1] There is no check between gasIsEth and bridgeToken.

There is a strict relationship between gasIsEth and bridgeToken. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/DecentBridgeAdapter.sol#L21-L24

constructor(bool _gasIsEth, address _bridgeToken) BaseAdapter() {
    gasIsEth = _gasIsEth;
    bridgeToken = _bridgeToken;
}

If the bridge token is set, the user should transfer this bridge token. If not, the user should send ETH. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L216

function approveAndCheckIfNative(
    BridgeInstructions memory instructions,
    uint256 amt2Bridge
) private returns (bool) {
    IBridgeAdapter bridgeAdapter = IBridgeAdapter(bridgeAdapters[instructions.bridgeId]);
    address bridgeToken = bridgeAdapter.getBridgeToken(
        instructions.additionalArgs
    );
    if (bridgeToken != address(0)) {
        IERC20(bridgeToken).approve(address(bridgeAdapter), amt2Bridge);
        return false;
    }
    return true;
}

In the Bridge Adapter, we assume that the adapter receives the bridge token when gasIsEth is false and receives ETH when gasIsEth is true. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/DecentBridgeAdapter.sol#L108-L115

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

Therefore, the following relationship should be satisfied. bridgeToken = weth if gasIsEth is false, 0 if gasIsEth is true

And gasIsEth should be the same as gasCurrencyIsEth in DecentEthRouter. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L22

contract DecentEthRouter is IDecentEthRouter, IOFTReceiverV2, Owned {
    bool public gasCurrencyIsEth;
}

[L-2] The transaction can fail when there is not enough DcntEth tokens in the DecentEthRouter contract.

When users attempt to swap tokens, they send WETH tokens to the router. The router receives WETH tokens and removes an equivalent amount of DcntEth tokens. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L177-L193

function _bridgeWithPayload (
    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
    );
}

So, if the router does not have enough DcntEth tokens, the transaction can fail. While anyone can deposit DcntEth to the router or the owner can mint tokens to the router, for safety, we should add the following.

function bridge() {
+    if (dcntEth.balanceOf(address(this)) < _amount) {
+        dcntEth.mint(address(this), _amount);
+    }
}

This is possible because the router has the ability to mint DcntEth tokens.

[L-3] Users can use the protocol without paying fees.

When users attempt to swap tokens using swapAndExecute or bridgeAndExecute, they should pay protocol fees. https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L115

function swapAndExecute(
    SwapAndExecuteInstructions calldata instructions,
    FeeStructure calldata fees,
    bytes calldata signature
)
    public
    payable
    retrieveAndCollectFees(fees, abi.encode(instructions, fees), signature)
{
}

However, users can also swap tokens using the receiveFromBridge function, and they are not required to pay protocol fees. 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);
}

[L-4] Missing payable

The execute function in UTBExecutor is not marked as payable. https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/UTBExecutor.sol#L49

function execute(
    address target,
    address paymentOperator,
    bytes memory payload,
    address token,
    uint amount,
    address payable refund,
    uint extraNative
) public onlyOwner {
}

[L-5] Unable to invoke the receiveFromBridge function from DecentBridgeExecutor properly.

If gasCurrencyIsEth and deliverEth are both true in the execute function, the _executeEth function is called. In this function, we perform a withdrawal of WETH and send ETH to the Bridge Adapter. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentBridgeExecutor.sol#L61

function _executeEth() {
    weth.withdraw(amount);
    (bool success, ) = target.call{value: amount}(callPayload);
}

However, in the receiveFromBridge function, we only accept ERC20 tokens. https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/bridge_adapters/DecentBridgeAdapter.sol#L139-L143

function receiveFromBridge() {
    IERC20(swapParams.tokenIn).transferFrom(
        msg.sender,
        address(this),
        swapParams.amountIn
    );
}

[L-6] There's a possibility that some funds may become trapped in the Bridge Adapter.

In the _bridgeWithPayload function, we set the Bridge Adapter as the refund recipient. https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L171

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

Consequently, certain funds will be accumulated as fees within the Bridge Adapter. There should be no remaining funds left in the Bridge Adapter in any case.