Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Decent smart contract system written in Solidity. The audit took place between January 19—January 23 2024.

Wardens

120 Wardens contributed reports to Decent:

1. windhustler
2. EV_om
3. NPCsCorp (0xStalin and sin1st3r__)
4. iamandreiski
5. haxatron
6. Soliditors (Tadev, 3docSec and 0xBeirao)
7. nuthan2x
8. deth
9. MrPotatoMagic
10. Aamir
11. peanuts
12. rouhsamad
13. monrel
14. nmirchev8
15. Topmark
16. wangxx2026
17. gesha17
18. bart1e
19. NentoR
20. imare
21. DadeKuma
22. SBSecurity (Slavcheww and Blckhv)
23. c3phas
24. fouzantanveer
25. slvDev
26. 0x11singh99
27. 0xSmartContract
28. yongskiws
29. 0xepley
30. SAQ
31. catellatech
32. Shaheen
33. Kaysoft
34. bronze_pickaxe
35. Tendency
36. Kow
37. ZdravkoHr
38. nobody2018
39. ether_sky
40. kutugu
41. Giorgio
42. 0xJaeger
43. Eeyore
44. hunter_w3b
45. Inference
46. 0xAadi
47. cu5t0mpeo
48. zaevlad
49. dharma09
50. Raihan
51. GhK3Ndf
52. ihtishamsudo
53. foxb868
54. clara
55. albahaca
56. dutra
57. Aymen0909
58. CDSecurity (chrisdior4 and ddimitrov22)
59. th13vn
60. pkqs90
61. 0xDING99YA
62. antonttc
63. Timepunk
64. ptsanev
65. Matue
66. 0xdedo93
67. SovaSlava
68. Pechenite (Bozho and radev_sw)
69. 0xmystery
70. rjs
71. IceBear
72. zxriptor
73. mgf15
74. ZanyBonzy
75. seraviz
76. 0xSimeon
77. azanux
78. 0xprinc
79. Timeless
80. slylandro_star
81. 0xdice91
82. Nikki
83. ke1caM
84. Greed
85. al88nsk
86. DarkTower (Gelato_ST, Maroutis, OxTenma, and 0xrex)
87. Timenov
88. ravikiranweb3
89. mrudenko
90. 0xBugSlayer
91. GeekyLumberjack
92. adeolu
93. stealth
94. simplor
95. PUSH0 (jojo and thekmj)
96. 0xabhay
97. darksnow
98. m4ttm
99. 0xE1
100. boredpukar
101. abiih
102. bareli
103. vnavascues
104. d4r3d3v1l
105. 0xPluto
106. Krace
107. kodyvim
108. Tigerfrake
109. JanuaryPersimmon2024
110. piyushshukla

This audit was judged by 0xsomeone.

Final report assembled by PaperParachute.

Summary

The C4 analysis yielded an aggregated total of 9 unique vulnerabilities. Of these vulnerabilities, 4 received a risk rating in the category of HIGH severity and 5 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 15 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 6 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Decent repository, and is composed of 11 smart contracts written in the Solidity programming language and includes 1209 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, vuln-detector from warden(s) oualidpro, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (4)

[H-01] Anyone can update the address of the Router in the DcntEth contract to any address they would like to set.

Submitted by NPCsCorp, also found by seraviz, dutra, 0xSimeon, EV_om, azanux, Aymen0909, 0xprinc, ZdravkoHr, 0x11singh99, CDSecurity, nuthan2x, GhK3Ndf, 0xAadi, Eeyore, ZanyBonzy (1, 2), DadeKuma, Matue, Timeless, Giorgio, slylandro_star, 0xdice91, Nikki, ke1caM, cu5t0mpeo, Greed, nobody2018, Tendency, Inference, al88nsk, DarkTower, th13vn, Soliditors, Timenov, wangxx2026, NentoR, ether_sky, peanuts, MrPotatoMagic, ravikiranweb3, mrudenko, Kaysoft, deth, 0xBugSlayer, nmirchev8, GeekyLumberjack, Aamir, adeolu, stealth, simplor, PUSH0, 0xabhay, darksnow, haxatron, m4ttm, 0xE1, boredpukar, abiih, 0xSmartContract, bareli, mgf15 (1, 2, 3, 4), vnavascues, d4r3d3v1l, zaevlad, 0xPluto, rouhsamad, Krace, kodyvim, Tigerfrake, JanuaryPersimmon2024, and piyushshukla

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

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.

0xsomeone (Judge) commented:

This and all relevant submissions correctly specify that the lack of access control in the DcntEth::setRouter function can be exploited maliciously and effectively compromise the entire TVL of the Decent ETH token.

A high-risk severity is appropriate, and this submission was selected as the best due to detailing all possible impacts:

• Arbitrary mints of the token to withdraw funds provided as liquidity to UTB
• Arbitrary burns to sabotage liquidity pools and other escrow-based contracts
• Sabotage of liquidity provision function invocations

wkantaros (Decent) confirmed

[H-02] Due to missing checks on minimum gas passed through LayerZero, executions can fail on the destination chain

Submitted by iamandreiski, also found by NPCsCorp, EV_om, windhustler (1, 2), and nuthan2x

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

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

In LayerZero, the destination chain’s function call requires a specific gas amount; otherwise, it will revert with an out-of-gas exception. It falls under the responsibility of the User Application to ensure that appropriate limits are established. These limits guide relayers in specifying the correct gas amount on the source chain, preventing users from inputting insufficient values for gas.

The contract logic in DecentEthRouter, assumes that a user will first get their estimated fees through estimateSendAndCallFee() and pass it as an argument in either bridge() or bridgeWithPayload() to be added to the calculation together with the hardcoded GAS_FOR_RELAY so that it can be passed as the adapter params when CommonOFT.LzCallParams is called, although this is not enforced and is left on the user’s responsibility.

A user can pass an arbitrary value as the _dstGasForCall argument to be added to the hardcoded GAS_FOR_RELAY fee, thus sending less gas than required which can lead to out-of-gas exceptions.

Once the message is received by destination, the message is considered delivered (transitioning from INFLIGHT to either SUCCESS or STORED), even though it threw an out-of-gas error.

Any uncaught errors/exceptions (including out-of-gas) will cause the message to transition into STORED. A STORED message will block the delivery of any future message from source to all destination on the same destination chain and can be retried until the message becomes SUCCESS.

As per: https://layerzero.gitbook.io/docs/faq/messaging-properties

Proof of Concept

According to the LayerZero integration checklist:
https://layerzero.gitbook.io/docs/troubleshooting/layerzero-integration-checklist

LayerZero recommends a 200,000 amount of gas to be enough for most chains and is set as default.

• “200k for OFT for all EVMs except Arbitrum is enough. For Arbitrum, set as 2M.”

In the DecentEthRouter, the GAS_FOR_RELAY is hardcoded at 100,000.

        uint256 GAS_FOR_RELAY = 100000;
        uint256 gasAmount = GAS_FOR_RELAY + _dstGasForCall;

The contract logic assumes that a user would willingly first call the estimateSendAndCallFee to get the nativeFee + the zroFee fom dcntEth.estimateSendAndCallFee and then pass the addition of the nativeFee + zeroFee as the _dstGasForCall argument when calling bridge() or 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
            );
    }

Once the internal _bridgeWithPayload function is called:

            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),
            zroPaymentAddress: address(0x0),
            adapterParams: adapterParams
        });

It calls the _getCallParams function which will calculate and pack the needed arguments to be passed as the LayerZero call params, without performing any checks whether the total gas amount is sufficient or that the user passed argument _dstGasForCall is greater than the total of (uint nativeFee, uint zroFee) = dcntEth.estimateSendAndCallFee.

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

This can lead to failed messages on the destination. Which would yield the above-message results of a possible blockage in the communication with the source and destination.

• A malicious or an unbeknownst user can pass 1,000 as an argument for _dstGasForCall, making the total gas forwarded to the LzCallParams() 101,000 which will make a certain type of calls fail (depending on payload), and especially calls made on Arbitrum.

Recommended Mitigation Steps

Validate/require that the _dstGasForCall parameter is greater than nativeFee + zroFee or re-engineer the architecture to make the estimateSendAndCallFee() function a mandatory step of the process.

0xsomeone (Judge) increased severity to High and commented:

This and all duplicate exhibits highlight that the GAS_FOR_RELAY is a hard-coded value and that the overall gas supplied for a cross-chain call can be controlled by a user.

A severity of high is appropriate given that the cross-chain LayerZero channel will be permanently blocked.

None of the submissions have correctly proposed a solution as a mere adjustment of the GAS_FOR_RELAY is insufficient. The DecentBridgeExecutor permits arbitrary calls to be made that can force the transaction to run out-of-gas regardless of the gas limit imposed. This is properly defined in #697.

A valid solution for this problem would be a combination of a minimum enforced at the transaction level and a maximum gas consumed enforced at the executor level, ensuring that the gas remainder after the executor performs the arbitrary call is enough to store the failed message. This can be achieved by performing a subtraction from the gasleft value (hard to implement as it would need to take into account the cost of keccak256 encoding the data payload) or by enforcing a fixed value that should be much less than the minimum imposed on the source chain.

This submission was selected as the best given that it illustrates in-depth knowledge of the LayerZero system states and correctly highlights that a user can also maliciously block the channel.

wkantaros (Decent) acknowledged via duplicate #212, but disagreed with severity and commented:

This vulnerability is not a concern in Layer Zero v2. Decent designed the contracts expecting to use LZ v2 and have since implemented this upgrade.

[H-03] When DecentBridgeExecutor.execute fails, funds will be sent to a random address

Submitted by DadeKuma, also found by NPCsCorp, MrPotatoMagic, SBSecurity, deth, nmirchev8, Tendency, ether_sky, Kow, haxatron, EV_om, 0xJaeger, ZdravkoHr, Giorgio, Soliditors, Aamir, Eeyore, Inference, and kutugu

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

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

When the DecentBridgeExecutor._executeWeth/_executeEth target call fails, a refund is issued to the from address.

However, this address is wrongly set, so those refunds will be permanently lost.

Proof of Concept

UTB.bridgeAndExecute (Link) calls DecentBridgeAdapter.bridge (Link), which calls DecentEthRouter.bridgeWithPayload (Link), where the payload is constructed (Link):

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

Inside _getCallParams the from address is the msg.sender, i.e. the DecentBridgeAdapter address on the source chain (Link):

	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, //@audit 'from' address
	            _toAddress,
	            deliverEth,
	            additionalPayload
	        );
	    }
	}

After the payload is constructed, DecentEthRouter sends the message:

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

Finally, on the destination chain, DecentEthRouter will receive the message (Link):

	function onOFTReceived(
	    uint16 _srcChainId,
	    bytes calldata,
	    uint64,
	    bytes32,
	    uint _amount,
	    bytes memory _payload
	) external override onlyLzApp {
		//@audit from is the decentBridgeAdapter address on the source chain
	    (uint8 msgType, address _from, address _to, bool deliverEth) = abi
	        .decode(_payload, (uint8, address, address, bool));
	    ...
	}

At the end of this function, the executor is invoked with the same _from address:

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

Finally, this is the execute function in DecentBridgeExecutor (Link):

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

Both _executeWeth and _executeEth have the same issue and funds will be lost when the target call fails, for example _executeEth (Link):

	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); //@audit wrong address as it uses the source address, not destination
	    }
	}

Now, DecentBridgeAdapter as a refund address seems wrong, as I disclosed in another issue, but let’s suppose that it isn’t, as it’s possible to prove both scenarios.

As proof by contradiction, funds should be sent to DecentBridgeAdapter, and this would be a non-issue if these contracts are deployed with CREATE2, as they would have the same address. But they are not deployed like that.

There is hard evidence that they have different addresses, for example, these are the addresses for DcntEth and DecentEthRouter in two different chains, which are already deployed:

• lib/decent-bridge/actual-deployments/latest/arbitrumAddresses.json

{
  "arbitrum_DcntEth": "0x8450e1A0DebF76fd211A03c0c7F4DdbB1eEF8A85",
  "done": true,
  "arbitrum_DecentEthRouter": "0x17479B712A1FE1FFaeaf155379DE3ad1440fA99e"
}

• lib/decent-bridge/actual-deployments/latest/optimismAddresses.json

{
  "DcntEth": "0x4DB4ea27eA4b713E766bC13296A90bb42C5438D0",
  "done": true,
  "DecentEthRouter": "0x57beDF28C3CB3F019f40F330A2a2B0e0116AA0c2"
}

If we take a look at the deploy script for DecentBridgeAdapter it also doesn’t use CREATE2, as there isn’t a factory:

	function deployDecentBridgeAdapter(
	    address utb,
	    address decentEthRouter,
	    address decentBridgeExecutor
	) internal returns (
	    DecentBridgeAdapter decentBridgeAdapter
	) {
	    string memory chain = vm.envString("CHAIN");
	    bool gasIsEth = gasEthLookup[chain];
	    address weth = wethLookup[chain];
	    address bridgeToken = gasIsEth ? address(0) : weth;

@>	    decentBridgeAdapter = new DecentBridgeAdapter(gasIsEth, bridgeToken);
	    decentBridgeAdapter.setUtb(utb);
	    decentBridgeAdapter.setRouter(decentEthRouter);
	    decentBridgeAdapter.setBridgeExecutor(decentBridgeExecutor);
	    UTB(payable(utb)).registerBridge(address(decentBridgeAdapter));
	}

So these funds will be sent to a random address in any case.

Recommended Mitigation Steps

The executor.execute call in DecentEthRouter.onOFTReceived should be changed to an appropriate address (e.g. the user refund address) instead of using _from:

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

0xsomeone (Judge) commented:

The Warden has detailed how the encoding of the cross-chain payload will use an incorrect _from parameter under normal operating conditions, leading to failed transfers at the target chain refunding the wrong address.

This submission was selected as the best given that it precisely details that the _from address is known to be incorrect at all times when the protocol is used normally.

A high-risk rating is appropriate as any failed call will lead to full fund loss for the cross-chain call.

wkantaros (Decent) confirmed

[H-04] Users will lose their cross-chain transaction if the destination router do not have enough WETH reserves.

Submitted by haxatron, also found by EV_om, MrPotatoMagic, deth, rouhsamad, Aamir, Topmark, and bart1e

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 != MTETHTRANSFER, 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.

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

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

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

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

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

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

wkantaros (Decent) confirmed but disagreed with severity

0xsomeone (Judge) commented:

This and all duplicate submissions detail an interesting way in which cross-chain relays will fail to properly invoke the target recipient of the relayed call, effectively leading to loss of funds as the assets will be transferred to an entity that potentially is not equipped to handle the token.

Based on discussions in #505, this is a very likely scenario and thus a high-risk severity is appropriate as the vulnerability should manifest consistently in non-Ethereum chains.

Medium Risk Findings (5)

[M-01] Permanent loss of tokens if swap data gets outdated

Submitted by windhustler, also found by monrel, nuthan2x, and imare

While sending funds through StargateBridgeAdapter, the user passes the swap data as a parameter. The flow is stargate sending tokens on the receiving chain into the StargateBridgeAdapter and executing sgReceive.

The issue here is that sgReceive will fail if the swap data gets outdated, but this is not going to make the whole transaction revert.

Stargate will still send the tokens to the StargateBridgeAdapter, and if the sgReceive fails, the swap will be cached for later execution. See StargateComposer logic here: https://stargateprotocol.gitbook.io/stargate/stargate-composability/stargatecomposer.sol#sgreceive.

Now, tokens will be left sitting in the StargateBridgeAdapter contract, and since the user can only retry the transaction with the same swap data, the tokens will be stuck forever.

The impact is loss of transferred tokens for the user.

Proof of Concept

Consider the following flow:

1. User invokes StargateBridgeAdapter:bridge() function as part of the whole flow of calling UTB:bridgeAndExecute() function.
2. He is passing the swapData parameters to the remote chain.
3. StargateComposer first transfers the tokens to the StargateBridgeAdapter contract.
4. After that the StargateBridgeAdapter:sgReceive() function is executed.
5. It then calls UTB:receiveFromBridge that tries to execute the swap.
6. If the swap fails, which can frequently occur due to the fact that the swap data is outdated, most commonly minimum amount out gets outdated.
7. The whole payload gets stored inside the StargateComposer contract, and the tokens are left in the StargateBridgeAdapter contract.
8. As the user can only retry the transaction with the same payload, the tokens will be stuck forever.
9. An example of StargateComposer contract can be seen on: https://stargateprotocol.gitbook.io/stargate/developers/contract-addresses/mainnet#ethereum

Note: The application needs to use the StargateComposer contract as opposed to StargateRouter because it is transferring funds with payload, i.e. it executes sgReceive on the receiving chain. You can only use StargateRouter directly if you’re sending funds empty payload. More on it: https://stargateprotocol.gitbook.io/stargate/stargate-composability

Recommended Mitigation Steps

Wrap the whole StargateBridgeAdapter:receiveFromBridge() call into a try/catch and if it reverts send the transferred tokens back to the user.

wkantaros (Decent) confirmed

0xsomeone (Judge) decreased severity to Medium and commented:

The Warden has demonstrated that it is possible for a Stargate-based cross-chain interaction to fail at the swap level perpetually.

While the StargateComposer::clearCachedSwap function exists to retry the same payload, as the Warden states, a sharp market event (i.e. token launch that leads to an upward trend or market crash that leads to a downward event) can cause the swap to fail.

Theoretically, the tokens can be rescued using a flash-loan if they are substantial, however, this would be very unconventional and not a real mitigation to the issue. The proposed solution by the Warden is adequate.

I believe a severity of Medium is more appropriate as the vulnerability relies on external requirements (i.e. a sharp market event), the maximum impact is the user’s own funds, and when users specify slippage (i.e. minimum output) they usually factor in the time it takes for a transaction to execute. Network congestion, market events, and cross-chain relay delays all play a role in whether the vulnerability manifests but present external requirements.

Note: For full discussion, see here.

[M-02] Users can use the protocol freely without paying any fees by calling the DecentEthRouter::bridgeWithPayload() function directly.

Submitted by NPCsCorp, also found by Soliditors, peanuts, nmirchev8, and haxatron

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

Note: to view the provided image, please see the original submission here.

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.

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

0xsomeone (Judge) decreased severity to Medium and commented:

The Warden has demonstrated that it is possible to bypass Decent fees when performing bridging operations by directly interacting with the DecentEthRouter. This is indeed a valid concern and has been confirmed by the Sponsor.

I believe a severity of medium is more appropriate given that uncaptured profit is solely affected.

wkantaros (Decent) confirmed

Note: For full discussion, see here.

[M-03] Missing access control on UTB:receiveFromBridge allows UTB swaps to be executed without spending bridge fees while bypassing fee/swap instruction signature verification

Submitted by GhK3Ndf, also found by dutra, CDSecurity, Aymen0909, 0xAadi, Eeyore, DadeKuma, pkqs90, th13vn, Soliditors, bart1e, NentoR, Kow, 0xDING99YA, antonttc, haxatron, Matue, 0xdedo93, SovaSlava, peanuts, and MrPotatoMagic

Users can abuse the public UTB:receiveFromBridge function’s lack of access control to directly call the internal UTB:_swapAndExecute function.

This bypasses the UTB:retrieveAndCollectFees modifier, which is used to collect fees from UTB users and validate fee and swap instructions via UTBFeeCollector:collectFees in all other public swap/bridge functions (namely UTB:swapAndExecute and UTB:bridgeAndExecute).

This allows users to execute inter-chain swaps without spending bridge fees, using instructions that have not been signed by a validator key. Unsigned additional payloads can also be included in the swap instruction’s payload element, which would then be executed by the UTBExecutor:execute function.

Root Cause

Missing access control in UTB:receiveFromBridge

UTB:receiveFromBridge is likely missing an access control modifier.

Note that the missing modifier may not be the [UTB:retrieveAndCollectFees] modifier, as this modifier is understood to be intended for checking the UTB swap/bridge instructions on the source chain. Refer to the remedial suggestions for an alternate means of access control.

UTB:receiveFromBridge

//File:src/UTB.sol

contract UTB is Owned {

    ...

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

    ...

    // if a feeCollector has been set, then this modifier validates the fee struct against the signer:

    modifier retrieveAndCollectFees(
        FeeStructure calldata fees,
        bytes memory packedInfo,
        bytes calldata signature
    ) {
        if (address(feeCollector) != address(0)) {
            uint value = 0;
            if (fees.feeToken != address(0)) {
                IERC20(fees.feeToken).transferFrom(
                    msg.sender,
                    address(this),
                    fees.feeAmount
                );
                IERC20(fees.feeToken).approve(
                    address(feeCollector),
                    fees.feeAmount
                );
            } else {
                value = fees.feeAmount;
            }
            feeCollector.collectFees{value: value}(fees, packedInfo, signature);
        }
        _;
    }

    ...
    
    // swapAndExecute/bridgeAndExecute functions both validate fees/instructions signatures via retrieveAndCollectFees modifier

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

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

    ...

    // _swapAndExecute directly bypasses any signature/fee checks before the swap/payload is executed.

    function _swapAndExecute(
        SwapInstructions memory swapInstructions,
        address target,
        address paymentOperator,
        bytes memory payload,
        address payable refund
    ) private {
        (address tokenOut, uint256 amountOut) = performSwap(swapInstructions);
        if (tokenOut == address(0)) {
            executor.execute{value: amountOut}(
                target,
                paymentOperator,
                payload,
                tokenOut,
                amountOut,
                refund
            );
        } else {
            IERC20(tokenOut).approve(address(executor), amountOut);
            executor.execute(
                target,
                paymentOperator,
                payload,
                tokenOut,
                amountOut,
                refund
            );
        }
    }

}

UTBFeeCollector:collectFees

//File:src/UTBFeeCollector.sol

    function collectFees(
        FeeStructure calldata fees,
        bytes memory packedInfo,
        bytes memory signature
    ) public payable onlyUtb {
        bytes32 constructedHash = keccak256(
            abi.encodePacked(BANNER, keccak256(packedInfo))
        );
        (bytes32 r, bytes32 s, uint8 v) = splitSignature(signature);
        address recovered = ecrecover(constructedHash, v, r, s);
        require(recovered == signer, "Wrong signature");
        if (fees.feeToken != address(0)) {
            IERC20(fees.feeToken).transferFrom(
                utb,
                address(this),
                fees.feeAmount
            );
        }
    }

UTBExecutor:execute

//File:src/UTBExecutor.sol:

contract UTBExecutor is Owned {
   
    ...

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

    ...

    function execute(
        address target,
        address paymentOperator,
        bytes memory payload,
        address token,
        uint amount,
        address payable refund,
        uint extraNative
    ) public onlyOwner {
        bool success;
        if (token == address(0)) {
            (success, ) = target.call{value: amount}(payload);
            if (!success) {
                (refund.call{value: amount}(""));
            }
            return;
        }

        uint initBalance = IERC20(token).balanceOf(address(this));

        IERC20(token).transferFrom(msg.sender, address(this), amount);
        IERC20(token).approve(paymentOperator, amount);

        if (extraNative > 0) {
            (success, ) = target.call{value: extraNative}(payload);
            if (!success) {
                (refund.call{value: extraNative}(""));
            }
        } else {
            (success, ) = target.call(payload);
        }

        uint remainingBalance = IERC20(token).balanceOf(address(this)) -
            initBalance;

        if (remainingBalance == 0) {
            return;
        }

        IERC20(token).transfer(refund, remainingBalance);
    }
}

Proof of Concept

The following Forge PoC test suite includes the following tests which demonstrates this issue, against a UTB/decent-bridge deployment on the Arbitrum and Polygon network chains, deployed via the provided helper functions in the repo test suite.

UTBReceiveFromBridge Forge test contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {UTB, SwapInstructions, SwapAndExecuteInstructions, FeeStructure} from "../src/UTB.sol";
import {UTBExecutor} from "../src/UTBExecutor.sol";
import {UniSwapper} from "../src/swappers/UniSwapper.sol";
import {SwapParams} from "../src/swappers/SwapParams.sol";
import {XChainExactOutFixture} from "./helpers/XChainExactOutFixture.sol";
import {UTBCommonAssertions} from "../test/helpers/UTBCommonAssertions.sol";
import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {IDcntEth} from "lib/decent-bridge/src/interfaces/IDcntEth.sol";
import {IDecentBridgeExecutor} from "lib/decent-bridge/src/interfaces/IDecentBridgeExecutor.sol";
import {IDecentEthRouter} from "lib/decent-bridge/src/interfaces/IDecentEthRouter.sol";
import {DcntEth} from "lib/decent-bridge/src/DcntEth.sol";
import {DecentBridgeExecutor} from "lib/decent-bridge/src/DecentBridgeExecutor.sol";
import {DecentEthRouter} from "lib/decent-bridge/src/DecentEthRouter.sol";
import {IUTB} from "../src/interfaces/IUTB.sol";
import {IUTBExecutor} from "../src/interfaces/IUTBExecutor.sol";
import {IUTBFeeCollector} from "../src/interfaces/IUTBFeeCollector.sol";
import {LzChainSetup} from "lib/forge-toolkit/src/LzChainSetup.sol";
import {console2} from "forge-std/Test.sol";
import {VmSafe} from "forge-std/Vm.sol";

contract UTBReceiveFromBridge is XChainExactOutFixture {

    UTB src_utb;
    UTB dst_utb;
    UniSwapper src_swapper;
    UniSwapper dst_swapper;
    DecentEthRouter src_DecentEthRouter;
    DecentEthRouter dst_DecentEthRouter;
    IDcntEth src_IDcntEth;
    address src_weth; 
    address src_usdc; 
    address dst_weth;
    address dst_usdc;
    uint256 slippage = 1;
    uint256 FEE_AMOUNT = 0.01 ether;
    CalledPOC ap;

    function setUp() public {
        src = arbitrum;
        dst = polygon;
        preSlippage = 2;
        postSlippage = 3;
        initialEthBalance = 1 ether;
        initialUsdcBalance = 10e6;
        MINT_GAS = 9e5;
        setRuntime(ENV_FORGE_TEST);
        loadAllChainInfo();
        setupUsdcInfo();
        setupWethHelperInfo();
        loadAllUniRouterInfo();
        setSkipFile(true);
        vm.label(alice, "alice");
        vm.label(bob, "bob");
        src_weth = getWeth(src);
        src_usdc = getUsdc(src);
        dst_weth = getWeth(dst);
        dst_usdc = getUsdc(dst);
        feeAmount = FEE_AMOUNT;
        _setupXChainUTBInfra();
        _srcChainSetup();
        // start all activities in src chain by default
        switchTo(src);
        ap = new CalledPOC();
    }

    function _setupXChainUTBInfra() internal {
        (src_utb,,src_swapper,src_DecentEthRouter,,) = deployUTBAndItsComponents(src);
        (dst_utb,,dst_swapper,dst_DecentEthRouter,,) = deployUTBAndItsComponents(dst);
        wireUpXChainUTB(src, dst);
    }

    function _srcChainSetup() internal {
        dealTo(src, alice, initialEthBalance);
        mintUsdcTo(src, alice, initialUsdcBalance);
        mintWethTo(src, alice, initialEthBalance);
        cat = deployTheCat(src);
        catUsdcPrice = cat.price();
        catEthPrice = cat.ethPrice();
    }

    function _setupAndGetInstructionsFeesSignature() internal returns(
        SwapAndExecuteInstructions memory,
        FeeStructure memory,
        bytes memory){
        (SwapParams memory swapParams, uint expected) = getSwapParamsExactOut(
            src,
            src_weth,
            src_usdc,
            catUsdcPrice,
            slippage
        );
        address payable refund = payable(alice);
        SwapInstructions memory swapInstructions = SwapInstructions({
            swapperId: src_swapper.getId(),
            swapPayload: abi.encode(swapParams, address(src_utb), refund)
        });
        startImpersonating(alice);
        ERC20(src_weth).approve(address(src_utb), swapParams.amountIn);
        SwapAndExecuteInstructions
            memory instructions = SwapAndExecuteInstructions({
                swapInstructions: swapInstructions,
                target: address(cat),
                paymentOperator: address(cat),
                refund: refund,
                payload: abi.encodeCall(cat.mintWithUsdc, (bob))
            });

        (   bytes memory signature,
            FeeStructure memory fees
        ) = getFeesAndSignature(instructions);
        stopImpersonating();
        return (instructions, fees, signature);
    }

    /*
    Testing correct UTB fee collection/signature validation during normal swap. 
    Adapted from UTBExactOutRoutesTest:testSwapWethToUsdcAndMintAnNft
    */
    function testUTBReceiveFromBridge_SwapWethToUsdcAndMintAnNFTWithFees() public {
        (SwapAndExecuteInstructions memory _instructions,
        FeeStructure memory _fees,
        bytes memory _signature) = _setupAndGetInstructionsFeesSignature();
        startImpersonating(alice);
        uint256 aliceETHBalanceBefore = address(alice).balance;
        src_utb.swapAndExecute{value: feeAmount}(_instructions, _fees, _signature);
        stopImpersonating();
        // confirm alice has spent feeAmount
        assertEq(address(alice).balance, aliceETHBalanceBefore - feeAmount);
        // confirm bob got the NFT
        assertEq(cat.balanceOf(bob), 1);
        assertEq(ERC20(src_usdc).balanceOf(address(cat)), cat.price());
        // checking fees
        address feeCollector = address(feeCollectorLookup[src]);
        if (feeToken == address(0)) {
            // expect src feeCollector balance to be the feeAmount
            assertEq(feeCollector.balance, feeAmount);
        } else {
            assertEq(ERC20(feeToken).balanceOf(feeCollector), feeAmount);
        }
    }

    /*
    Missing access control on UTB:receiveFromBridge allows UTB swaps to be executed while bypassing fee/swap instruction signature verification. 
    */
    function testUTBReceiveFromBridge_BypassFeesAndSignature() public {
        /* getting the SwapAndExecuteInstructions struct for 
        swapping WETH to USDC and minting bob a VeryCoolCat NFT.
        FeeStructure and signature are not necessary this time.
        */
        (SwapAndExecuteInstructions memory _instructions,,) = _setupAndGetInstructionsFeesSignature();
        // checking feeCollector to see if it has receievd any fees
        address feeCollector = address(feeCollectorLookup[src]);
        uint256 aliceETHBalanceBefore = address(alice).balance;
        uint256 feeCollectorETHBalanceBefore = address(feeCollector).balance;
         startImpersonating(alice);
        /* use UTB:receiveFromBridge to directly call UTB:_swapAndExecute,
         bypassing UTB:retrieveAndCollectFees modifier to send tx without fees or signature
         with arbitrary additional payload.*/
        src_utb.receiveFromBridge(
            _instructions.swapInstructions,
            _instructions.target,
            _instructions.paymentOperator,
            _instructions.payload,
            _instructions.refund);
        stopImpersonating();
        // confirm alice has not spent any ETH/fees
        assertEq(address(alice).balance, aliceETHBalanceBefore);
        // confirm bob got the NFT
        assertEq(cat.balanceOf(bob), 1);
        assertEq(ERC20(src_usdc).balanceOf(address(cat)), cat.price());
        if (feeToken == address(0)) {
            // expect src feeCollector ETH balance not to change. In this case, it is 0
            assertEq(feeCollector.balance, feeCollectorETHBalanceBefore); 
            assertEq(feeCollector.balance, 0); 
        } else {
            assertEq(ERC20(feeToken).balanceOf(feeCollector), 0);
        }
    }

    /* Showing arbitrary calldata being executed in SwapAndExecuteInstructions.payload 
    by using UTB:receiveFromBridge to send an unsigned swap for 0 amount with no fees*/
    function testUTBReceiveFromBridge_ArbitraryCalldata() public {
        // arg: SwapInstructions memory postBridge
        (SwapParams memory swapParams, uint expected) = getSwapParamsExactOut(
            src, // string memory chain
            src_weth, // address tokenIn/tokenOut can be the same.
            src_weth, // address tokenOut
            0, // uitn256 amountOut can be zero
            slippage // uint256 slippage
        );
        // arg: address payable refund
        address payable refund = payable(alice); 
        // get SwapInstructions for SwapAndExecuteInstructions
        SwapInstructions memory swapInstructions = SwapInstructions({
            swapperId: src_swapper.getId(),
            swapPayload: abi.encode(swapParams, address(src_utb), refund)
        });
        startImpersonating(alice);
        //get SwapAndExecuteInstructions
        SwapAndExecuteInstructions
        memory _instructions = SwapAndExecuteInstructions({
            swapInstructions: swapInstructions,
            target:address(ap), // will be sending funds to alice on DST
            paymentOperator: address(alice), // address UTBExecutor approves
            refund: payable(address(alice)),
            // make arbitrary call
            payload: abi.encodeCall(ap.called, ())
        });
        // start recording for events
        vm.recordLogs();
        /* use UTB:receiveFromBridge to directly call UTB:_swapAndExecute,
        bypassing UTB:retrieveAndCollectFees modifier to send tx without fees or signature
        with arbitrary additional payload.*/
        src_utb.receiveFromBridge(
            _instructions.swapInstructions,
            _instructions.target,
            _instructions.paymentOperator,
            _instructions.payload,
            _instructions.refund);
        stopImpersonating();
        // capture emitted events
        VmSafe.Log[] memory entries = vm.getRecordedLogs();
        for (uint i = 0; i < entries.length; i++) {
        if (entries[i].topics[0] == keccak256("Called(string)")) {
            // assert that the event data contains the POC contract's string.
            assertEq(abi.decode(entries[i].data, (string)), 
            "POC contract called");
            }   
        }
    }
}

// simple test contract to register an event if called() is called.
contract CalledPOC {

    event Called(string);

    constructor() payable {}
    function called() public payable {
        emit Called("POC contract called");
    }

    receive() external payable {}

    fallback() external payable {}
}

Tests:

• testUTBReceiveFromBridge_SwapWethToUsdcAndMintAnNFTWithFee: A benign test function to ensure normal swaps with fees are working as intended, where Alice executes a signed inter-chain swap between WETH and USDC before minting a VeryCoolCat test NFT to Bob. Adapted from the UTBExactOutRoutesTest:testSwapWethToUsdcAndMintAnNft test.
• testUTBReceiveFromBridge_BypassFeesAndSignature: Demonstrating the issue by using the same swap with fee payload used in the testUTBReceiveFromBridge_SwapWethToUsdcAndMintAnNFTWithFee test, sent through the UTB:receiveFromBridge function.
• testUTBReceiveFromBridge_ArbitraryCalldata: Demonstrating the issue by causing arbitrary unsigned calldata to be executed via the UTB:receiveFromBridge function. Note that a swap between the same WETH token, for amount 0, was used as the swap instructions for this test for clarity.

PoC instructions:

• Clone and set up the audit repo repo and .env file as noted in Tests
• Add the Forge test suite file, UTBReceiveFromBridge.t.sol, to the repo’s /test directory.
• Run the following command to validate all tests:

❯ forge test --match-test testUTBReceiveFromBridge_
[⠒] Compiling...No files changed, compilation skipped
[⠢] Compiling...

Running 3 tests for test/UTBReceiveFromBridge.t.sol:UTBReceiveFromBridge
[PASS] testUTBReceiveFromBridge_ArbitraryCalldata() (gas: 169476)
[PASS] testUTBReceiveFromBridge_BypassFeesAndSignature() (gas: 678548)
[PASS] testUTBReceiveFromBridge_SwapWethToUsdcAndMintAnNFTWithFees() (gas: 703800)
Test result: ok. 3 passed; 0 failed; 0 skipped; finished in 24.86s
 
Ran 1 test suites: 3 tests passed, 0 failed, 0 skipped (3 total tests)

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

Primary Recommendation: Implement a robust access control modifier on the UTB:receiveFromBridge function to restrict access to known bridge adaptor addresses

The UTB:receiveFromBridge function appears to be intended to be called by the DecentBridgeAdapter:receiveFromBridge and StargateBridgeAdapter:sgReceive functions. These functions are protected by the BaseAdapter:onlyExecutor modifier. No other calls to [UTB:receiveFromBridge] are made in the audit codebase.

It therefore may be suitable to introduce a similar onlyBridgeAdapter modifier to UTB, using the already present UTB:bridgeAdapters mapping to filter calls from only allowlisted bridge adaptors:

    //File:src/UTB.sol
    ...

    modifier onlyBridgeAdapter(){
        require(bridgeAdapters[IBridgeAdapter(msg.sender).getId()] != address(0), "invalid bridge adaptor");
        _;
    }

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

Best practices: Review off-chain validator signature generation and update UTBFeeCollector:collectFees to allow for on-chain validation of signatures if UTB:feeCollector has not been set.

Note that in the receiveFromBridge modifier, fee and swap instructions are only validated if a feeCollector is set in UTB.

UTB:retrieveAndCollectFees

//File:src/UTB.col

    modifier retrieveAndCollectFees(
        FeeStructure calldata fees,
        bytes memory packedInfo,
        bytes calldata signature
    ) {
        if (address(feeCollector) != address(0)) { 
            
            // if feeCollector has not been set, then signature verifcation does not occur

            ...

            feeCollector.collectFees{value: value}(fees, packedInfo, signature);
        }
        _;
    }

UTBFeeCollector:collectFees

//File:src/UTBFeeCollector.sol

    function collectFees(
        FeeStructure calldata fees,
        bytes memory packedInfo,
        bytes memory signature
    ) public payable onlyUtb {
        bytes32 constructedHash = keccak256(
            abi.encodePacked(BANNER, keccak256(packedInfo))
        );
        (bytes32 r, bytes32 s, uint8 v) = splitSignature(signature); // validating both fees and swap instructions with signature from signer
        address recovered = ecrecover(constructedHash, v, r, s);
        require(recovered == signer, "Wrong signature");

        ...
    }

It is not known whether this is an intended design choice, possibly stemming from the way any off-chain swap/bridge/fee instruction validator APIs generate signatures and the fact that one signature is expected for both fee content and swap/bridge instructions.

However, if swaps/bridge operations are intended to ever be called without incurring a fee via UTBFeeCollector, it is recommended for off-chain APIs to generate swap/bridge operation signatures with a fee amount of 0 in the case where fees are not intended to be collected for signed swap/bridge operations.

This would allow the feecollector.collectFees line in UTB to be placed outside of the if check on the feeCollector address.

Alternatively, it is recommended to separate signature verification of fee and swap/bridge instructions in both the API and the UTBFeeCollector contract, to allow their associated signatures to be validated independently.

This would ensure that signed swap/bridge instructions can be verified before execution, in the event that a UTBFeeCollector contract is not set for a particular chain/UTB deployment.

0xsomeone (Judge) decreased severity to Medium and commented:

This and its duplicate submissions have illustrated that the UTB::receiveFromBridge will behave identically to the UTB::swapAndExecute function albeit with no fees applied or signatures validated, permitting users to bypass these measures.

The maximum impact of this and relevant submissions is the loss of fees that are meant to be imposed when the UTB::swapAndExecute functionality is utilized. As a subset of submissions has recognized, the signature validation is also bypassed permitting arbitrary payloads to be executed which is also considered unwanted with an unquantifiable impact.

Given that the UTB::swapAndExecute is one of the main features of the protocol, I consider a medium-risk severity for this exhibit to be more appropriate.

This submission has been selected as the best due to going in great detail in relation to the submission, however, it should be noted that the recommended alleviation suffers from impersonation attacks (i.e. results from getter functions on the msg.sender can be spoofed) and a mapping based whitelist mechanism should be enforced instead.

wkantaros (Decent) confirmed

[M-04] Potential loss of capital due to fixed fee calculations

Submitted by Soliditors, also found by Soliditors, windhustler (1, 2), gesha17, wangxx2026, NentoR, and peanuts

The StargateBridgeAdapter relies on a fixed fee calculation (0.06% of the current Stargate fee), but as explained in the Stargate documentation, fees can be automatically adjusted to meet demand. (here)

This reward can be adjusted (StargateFeeLibraryV02.sol#L68) to “To incentivize users to conduct swaps that ‘refill’ native asset balances”. A problem arises because the StargateBridgeAdapter doesn’t account for this variable fee.

Then the callback function (triggered on the target chain) will receive a token amount greater than amountIn. StargateBridgeAdapter.sol#L207

IERC20(swapParams.tokenIn).approve(utb, swapParams.amountIn);

As you can see, here the difference between the received amount StargateBridgeAdapter.sol#L188 and swapParams.amountIn gets lost in the adapter.

Recommended Mitigation Steps

It’s recommended approve the amountLD instead of the swapParams.amountIn . This way, all token received during the callback will be transfered.

function sgReceive(
    uint16, // _srcChainid
    bytes memory, // _srcAddress
    uint256, // _nonce
-  	address, // _token
-   uint256, // amountLD
+   address _token,
+   uint256 amountLD,
    bytes memory payload
) external override onlyExecutor {
    (
        SwapInstructions memory postBridge,
        address target,
        address paymentOperator,
        bytes memory utbPayload,
        address payable refund
    ) = abi.decode(
            payload,
            (SwapInstructions, address, address, bytes, address)
        );

    SwapParams memory swapParams = abi.decode(
        postBridge.swapPayload,
        (SwapParams)
    );
- IERC20(swapParams.tokenIn).approve(utb, swapParams.amountIn);
+ IERC20(_token).approve(utb, amountLD); // _token == swapParams.tokenIn

+ swapParams.amountIn = amountLD // swapParams also needs to be updated to swap the correct amount
+ postBridge.swapPayload = abi.encode(swapParams);

    IUTB(utb).receiveFromBridge(
        postBridge,
        target,
        paymentOperator,
        utbPayload,
        refund
    );
}

[M-05] DecentEthRouter.sol#_bridgeWithPayload() - Any refunded ETH (native token) will be refunded to the DecentBridgeAdapter, making them stuck

Submitted by deth, also found by NPCsCorp, MrPotatoMagic, Shaheen, ZdravkoHr, DadeKuma, gesha17, cu5t0mpeo, wangxx2026, zaevlad, Tendency, haxatron, bronze_pickaxe, nmirchev8, kutugu, Timepunk, and ptsanev

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