Decent - NPCsCorp's results

Lines of code

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

Vulnerability details

Impact

By allowing anybody to set the address of the Router contract to any address they want to set it allows malicious users to get access to the mint and burn functions of the DcntEth contract.

Proof of Concept

The DcntEth::setRouter() function has not an access control to restrict who can call this function. This allows anybody to set the address of the router contract to any address they'd like to set it.

DcntEth.sol

//@audit-issue => No access control to restrict who can set the address of the router contract
function setRouter(address _router) public {
    router = _router;
}

The functions DcntEth::mint() function & DcntEth::burn() function can be called only by the router contract.

DcntEth.sol

    //@audit-info => Only the router can call the mint()
    function mint(address _to, uint256 _amount) public onlyRouter {
        _mint(_to, _amount);
    }

    //@audit-info => Only the router can call the burn()
    function burn(address _from, uint256 _amount) public onlyRouter {
        _burn(_from, _amount);
    }

A malicious user can set the address of the router contract to an account of their own and:

1. Gain access to mint unlimited amounts of DcntEth token, which could be used to disrupt the crosschain accounting mechanism, or to steal the deposited weth in the DecentEthRouter contract.
2. Burn all the DcntEth tokens that were issued to the DecentEthRouter contract when liquidity providers deposited their WETH or ETH into it.
3. Cause a DoS to the add and remove liquidity functions of the DecentEthRouter contract. All of these functions end up calling the DcntEth::mint() function or the DcntEth::burn() function, if the router address is set to be different than the address of the DecentEthRouter, all the calls made from the DecentEthRouter to the DcnEth contract will revert.

DecentEthRouter.sol

    /// @inheritdoc IDecentEthRouter
    function addLiquidityEth()
        public
        payable
        onlyEthChain
        userDepositing(msg.value)
    {
        weth.deposit{value: msg.value}();
        
        //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.mint(address(this), msg.value);
    }

    /// @inheritdoc IDecentEthRouter
    function removeLiquidityEth(
        uint256 amount
    ) public onlyEthChain userIsWithdrawing(amount) {

      //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.burn(address(this), amount);
        weth.withdraw(amount);
        payable(msg.sender).transfer(amount);
    }

    /// @inheritdoc IDecentEthRouter
    function addLiquidityWeth(
        uint256 amount
    ) public payable userDepositing(amount) {
        weth.transferFrom(msg.sender, address(this), amount);

        //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.mint(address(this), amount);
    }

    /// @inheritdoc IDecentEthRouter
    function removeLiquidityWeth(
        uint256 amount
    ) public userIsWithdrawing(amount) {

      //@audit-issue => If router in the dcnteth contract is not set to the address of the DecentEthRouter, this call will revert
        dcntEth.burn(address(this), amount);
        weth.transfer(msg.sender, amount);
    }

Tools Used

Manual Audit

Recommended Mitigation Steps

Make sure to add an Acess Control mechanism to limit who can set the address of the Router in the DcnEth contract.

Assessed type

Access Control

Lines of code

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

Vulnerability details

The vulnerability

The root cause of the vulnerability exists in the DecentEthRouter::_getCallParams() internal function which is responsible for configuring the adapter parameters which will be sent to Layerzero

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 DecentEthRouter::_getCallParams() function takes the value of the parameter _dstGasForCall (which is user supplied) and adds it to the pre-set amount of gas for the relayer (100,000) and sets the result as the amount of gas to be sent.

The issue is that the attacker can supply zero as the value of _dstGasForCall parameter, leading to only 100,000 gas being set which is not sufficient and will result in StoredPayload and the message pathway getting blocked.

The user gets to supply the value of _dstGasForCall parameter at the very beginning when he decides to call the function UTB::bridgeAndExecute() in bridgeInstructions.additionalArgs, and then it keeps getting passed until it reaches DecentEthRouter::bridgeWithPayload which calls the internal function DecentEthRouter:_bridgeWithPayload() which will call the DecentEthRouter::_getCallParams() which will prepare the adapter parameters for the call to be sent to LayerZero

Additionally, the attacker can instruct the protocol to conduct an cross-chain arbitrary call to a malicious gas-griefing contract of his which will deplete all of the gas supplied leading the LZ call to fail and getting the message pathway to be be blocked.

Impact

The communication channel between a source chain and a destination chain can be blocked by exploiting the ability to specify arbitrary gas parameters for the destination chain function calls. An attacker consistently (and at a low cost), can block communication between the source chaind and a destination chain (which can only be unblocked through a manual intervention)

Tools Used

Manual Audit & LayerZero

Recommended Mitigation Steps

This is most easily fixed by checking if the gas passed in the instructions.additionalArgs in the UTB::bridgeAndExecute() function is less than the minimum threshold.

Or fix it in the DecentEthRouter::_getCallParams() function and check if the _dstGasForCall meets the minimum amount of gas needed.

Found & reported by: sin1st3r__

Team: NPCsCorp

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users will lost all their bridged funds in case that the bridge execution fails in the destination chain.

Proof of Concept

When a user is bridging tokens to perform an execution in the destination chain, the users initiates their transaction by:

1. Calling the UTB::bridgeAndExecute() function, from there it will do the swap pre-bridge (in the src chain) & will modify the swapInstructions for the swap post-bridge (in the dest chain), then it will invoke the UTB::callBridge() function
2. In the UTB::callBridge() function some approvals are granted to the DecentBridgeAdapter contract, and then the execution is forwarded to the DecentBridgeAdapter::bridge() function
3. In the DecentBridgeAdapter::bridge() function, some data is encoded and decoded to prepare the calldata for the bridge, and then the DecentEthRouter::bridgeWithPayload() function is called.
4. When the execution reaches the DecentEthRouter::bridgeWithPayload() function, notice how the msg.sender in the DecentEthRouter contract will be the DecentBridgeAdapter contract, (from step 3 to step 4, the caller is the DecentBridgeAdapter contract). When the payload that is sent for the execution in the destination chain is defined in the DecentEthRouter::_getCallParams() function, which is called by the DecentEthRouter::_bridgeWithPayload() function, the second parameter that is encoded as part of the payload variable it is set to be the msg.sender, keep in mind that the msg.sender is the address of the DecentBridgeAdapter, not the user's address or an address controlled by him.
5. From here onwards, the execution is sent to LayerZero, which will send the payload variable to the DecentEthRouter::onOFTReceived() function in the destination chain.

6. When the DecentEthRouter::onOFTReceived() function is called, the received _payload is decoded and the encoded values are saved in individual variables,*** the one we are interested is the second variable***, which is the msg.sender that was encoded in the DecentEthRouter::_getCallParams() function, then, if the crosschain message happens to be of an execute type, the execution will be forwarded to the DecentBridgeExecutor::execute() function
7. In the DecentBridgeExecutor::execute() function, either the DecentBridgeExecutor::_executeWeth() function or the DecentBridgeExecutor::_executeEth() function are called, and the received from parameter (which was just decoded from the received payload in the onOFTReceived()) is forwarded to those functions.

8. In any of those functions, the execution will call the target contract (which is the DecentBridgeAdapter::receiveFromBridge() of the current chain).
9. If the call to the target contract reverts, all the bridged funds are refunded to the from parameter, which is exactly the msg.sender that was encoded in the DecentEthRouter::_getCallParams() function. (See step 4)

• Refunding to the msg.sender who called the DecentEthRouter::_getCallParams() function in the source chain will cause that the users lost all his bridged funds, and instead, the DecentBridgeAdapter contract address of the source chain will be the one that received the refund of the bridged funds.

Tools Used

Manual Audit

Recommended Mitigation Steps

Instead of hardcoding the refundAddress to be the msg.sender, use the refundee address that was already provided by the user. This address was passed in the calldata sent to the UTB::bridgeAndExecute() function, specifically, the refundAddress specified by the user is encoded in the instructions.refund variable.

• Make sure to decode the encoded calldata that gets to the DecentEthRouter::_getCallParams() function. Read the value of the instructions.refund variable that was sent at the beginning of the execution, and use that address to set the second variable of the payload variable that is passed as a parameter for the execution in the destination chain.

Assessed type

en/de-code

Lines of code

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

Vulnerability details

The execution flow of bridgeAndExecute function

To understand the vulnerability, we need to understand the execution flow of the bridgeAndExecute() function, at least a small portion of it.

When the user wants to bridge tokens of him and execute an action on another chain, he will need to execute the UTB::bridgeAndExecute() function.

Suppose the user exists in Polygon, he has USDC and he wants to mint an NFT in Optimism which costs 1000 DAI. What will happen is that the protocol will first, in polygon, swap the user's USDC with WETH, then bridge the WETH to Optimism, then swap the WETH with DAI and then execute the arbitrary call the user wants to execute, which will be to mint the NFT in exchange for the resulting 1000 DAI from the post-bridge swap operation.

When this function is called, the following will happen:

1. Step 1: When the user calls the UTB::bridgeAndExecute() function, it will do three things: first, it will collect the fees by calling the UTBFeeCollector:collectFees() function, secondly, it will conduct the pre-bridge swap operation (occurs in the source destination), it will swap the user's USDC to WETH. thirdly, it will modify the swapInstructions which the user supplied to prepare for the post-bridge swap. Then after all of the 3 operations take place, it will invoke the UTB::callBridge() function.

2. Step 2: In the UTB::callBridge() function, some approvals are granted to the DecentBridgeAdapter contract, and then the it will invoke the function DecentBridgeAdapter::bridge() in the DecentBridgeAdapter contract.

3. Step 3: In the DecentBridgeAdapter::bridge() function, some data like the post-bridge swap payload and bridge payload (what to execute when the TX reaches destination) will be encoded, then it will reach out to the DecentEthRouter contract and invoke the function DecentEthRouter::bridgeWithPayload

4. Step 4: When the execution reaches the DecentEthRouter::bridgeWithPayload function, an internal function containing the actual logic, with the same name will also be called: DecentEthRouter::_bridgeWithPayload

   Note: Notice that the `DecentEthRouter::bridgeWithPayload() function isn't protected by any modifiers, any body can call it directly

5. Step 5: When the execution gets inside the DecentEthRouter::_bridgeWithPayload function, the function will prepare the LzCallParams for the layerzero call and the actual bridging will happen when the dcntEth::sendAndCall function is actually invoked.

6. Step 6: The bridging process kickstarts and the execution flow is continued in the destination chain.

Here is a graph of the execution flow

The vulnerability & PoC

The vulnerability is that any user can conduct the pre-bridge swap operation using uniswap by himself, prepare the right calldata and call the function DecentEthRouter::bridgeWithPayload directly (since anybody can call it), doing so will allow the user to bypass the fee collection process completely.

The fee collection as mentioned in the previously detailed execution flow, happens when the user first calls the bridgeAndExecute() function. But as I mentioned, nothing really forces him to start execution from there. All that function does is collect the fees, conduct the pre-bridge swap operation and prepare the proper call data. The user can conduct the pre-bridge swap operation himself, prepare the proper calldata and talk directly to the DecentEthRouter::bridgeWithPayload function, effecitvely bypassing fee collection.

Anybody can call the DecentEthRouter::bridgeWithPayload directly, and the protocol has no mechanism of determining whether or not the user is using the protocol with or without paying fees.

Impact

Users can use the protocol without paying any fees.

Tools Used

Manual Audit

Recommended Mitigation Steps

Tighten up the access control on the DecentEthRouter::bridgeWithPayload function. Allow only the DecentBridgeAdapter to call the bridgeWithPayload() function.

Found & reported by: sin1st3r__

Team: NPCsCorp

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users will lose any excess gas they pay for LZ calls because the refundee address is incorrectly hardcoded to be the msg.sender of the DecentEthRouter::bridgeWithPayload() function rather than the refund address the user specified

Proof of Concept

When a crosschain message is sent through LayerZero it is required to prepare a calldata that it'll be sent to the LayerZero contracts. One of the parameter for this calldata it is the refundAddress, which indicates to the LayerZero contracts where to refund the excess gas that was paid for the transaction in case that the transaction's cost is cheaper than the amount of value passed.

This is the execution flow when a user wants to perform a bridge of funds from one chain to another

1. The user starts the execution by calling the a UTB::bridgeAndExecute() function, here the fees for the operation are charged, a swap pre-bridge its executed && the swapParams for the swap post-bridge are prepared, then execution goes to the UTB::callBridge() function
2. In the UTB::callBridge() function, some approvals are granted to the DecentBridgeAdapter contract, and then the execution is forwarded to the DecentBridgeAdapter::bridge() function
3. In the DecentBridgeAdapter::bridge() function, some data is encoded and decoded to prepare the calldata for the bridge, and then the DecentEthRouter::bridgeWithPayload() function is called.
4. When the execution reaches the DecentEthRouter::bridgeWithPayload() function, the msg.sender is be the DecentBridgeAdapter contract's address, (from step 3 to step 4, the caller is the DecentBridgeAdapter contract), when the LZCallParams are defined in the DecentEthRouter::_bridgeWithPayload() function, the callParams.refundAddress is set to msg.sender.

• By setting the callParams.refundAddress to be the msg.sender, what is happening is that the DecentBridgeAdapater contract is set as the refundAddress for the excess gas that was paid for the execution, instead of setting the refundAddress to be an address controller by the user.
  • This will cause that the DecentBridgeAdapter contract receives the refund of excess gas instead of an account controlled by the user who initiated the bridge operation.
    • The DecentBridgeAdapter contract will accumulate a balance, and the user losses the excess gas that was paid for the lz call, which means that instead of refunding the excess gas to an account controlled by the user, any excess gas in the destination chain will be refunded to the address of the DecentBridgeAdapater contract.

5. From the DecentEthRouter::_bridgeWithPayload() function, the execution is sent to the DcntEth::sendAndCall() function where it will end up calling the LayerZeroEndpoint::send() function.

6. Once the execution reaches the LayerZeroEndpoint contract, at the end of the execution of the LayerZeroEndpoint::send() function, the endpoint contract computes if there is any leftover gas that was paid but was not spent, if so, it refunds the excess gas to the _refundAddress that was passed in the callParams.refundAddress. The problem is that the callParams.refundAddress was encoded as the msg.sender who called the DecentEthRouter::bridgeWithPayload() function [Step 4]. This will cause that the refunded excess gas is sent to the DecentBridgeAdapater contract instead of an account controlled by the user.

Tools Used

Manual Audit, LayerZero Documentation & LayerZeroEndpoint contract

Recommended Mitigation Steps

Instead of hardcoding the refundAddress to be the msg.sender, use the refundee address that was already provided by the user. This address was passed in the calldata sent to the UTB::bridgeAndExecute() function, specifically, the refundAddress specified by the user is encoded in the instructions.refund variable.

• Make sure to decode the encoded calldata that gets to the DecentEthRouter::bridgeWithPayload() function. Read the value of the instructions.refund variable that was sent at the beginning of the execution, and use that address to set the callParams.refundAddress which specifies the address where the excess gas paid to LayerZero should be refunded.

Assessed type

en/de-code