Decent - DadeKuma's results

Sensitive disclosure

I'm submitting this issue as a sensitive disclosure because both of these contract are already deployed on multiple chains. For example, on Mainnet:

Router: 0x4B3876a8E3Bb072787F51c39be83d7cB0b4e6388
DecentEth: 0x4B3876a8E3Bb072787F51c39be83d7cB0b4e6388

Right now it's not possible to steal funds as the router doesn't have any WETH, but these contracts seem active.

Moreover, it's still possible to accumulate DecentEth right now and redeem WETH later on.

Impact

Anyone can change decentEthRouter as DcntEth.setRouter lacks any access control.

This can be used to access privileged functions such as DcntEth.mint and DcntEth.burn to manipulate the DcntEth supply.

This will cause a loss of funds as WETH can be redeemed by transferring DcntEth in the original router.

Proof of Concept

This function lacks access control, so it's callable by anyone:

/**
 * @param _router the decentEthRouter associated with this eth
 */
function setRouter(address _router) public {
    router = _router;
}

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

Attack steps:

1. Attacker waits until DecentEthRouter has some WETH liquidity
2. Attacker calls DcntEth.setRouter and changes it to a contract they own, so that they can call DcntEth.mint:

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

3. Attacker calls DecentEthRouter.redeemWeth in the original router to drain its funds:

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

Tools Used

Manual review

Recommended Mitigation Steps

Add a onlyOwner modifier to DcntEth.setRouter.

Lines of code

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

Vulnerability details

Impact

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.

Tools Used

Manual review

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

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTB.sol#L108-L124 https://github.com/code-423n4/2024-01-decent/blob/main/src/UTB.sol#L311-L319

Vulnerability details

Impact

When users perform a swap through UTB.swapAndExecute, they have to pay some fees, which are collected through the retrieveAndCollectFees modifier.

However, users can avoid paying those fees by spoofing a bridge receive call.

Proof of Concept

Normally, a user should call UTB.swapAndExecute (Link) to perform a swap, and fees are paid and collected through retrieveAndCollectFees:

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

However, a user can simply call UTB.receiveFromBridge to perform a swap without paying any fees, as it lacks access control (Link):

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding access control in the receiveFromBridge function, which should be callable only by a bridge adapter.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTB.sol#L259 https://github.com/code-423n4/2024-01-decent/blob/main/src/UTB.sol#L289 https://github.com/code-423n4/2024-01-decent/blob/main/src/bridge_adapters/DecentBridgeAdapter.sol#L117 https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L148

Vulnerability details

Impact

The LZ refundAddress is not set to a user controlled address, as all refunds will be sent to DecentBridgeAdapter.

This contract is not meant to hold any funds, and there is no way to extract them, so they will be permanently locked.

Proof of Concept

This is the bridging flow:

1. User calls UTB.bridgeAndExecute (Link)
2. UTB.bridgeAndExecute calls BridgeAdapter.bridge (Link)
3. BridgeAdapter.bridge calls DecentEthRouter.bridgeWithPayload (Link)

If we look at the _bridgeWithPayload function (Link):

	function _bridgeWithPayload(
	    uint8 msgType,
	    uint16 _dstChainId,
	    address _toAddress,
	    uint _amount,
	    uint64 _dstGasForCall,
	    bytes memory additionalPayload,
	    bool deliverEth
	) internal {
	    ...
	    ICommonOFT.LzCallParams memory callParams = ICommonOFT.LzCallParams({
@>	        refundAddress: payable(msg.sender),
	        zroPaymentAddress: address(0x0),
	        adapterParams: adapterParams
	    });

The refund address will be set to the BridgeAdapter address, as it was the last caller.

The adapter is not meant to hold any funds, so every time a refund occurs, those funds will be lost/locked inside the adapter.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding an explicit refund address as a function parameter:

	function _bridgeWithPayload(
	    uint8 msgType,
	    uint16 _dstChainId,
	    address _toAddress,
	    uint _amount,
	    uint64 _dstGasForCall,
	    bytes memory additionalPayload,
	    bool deliverEth,
+	    address payable refundAddress
	) internal {
		...
	    ICommonOFT.LzCallParams memory callParams = ICommonOFT.LzCallParams({
-	        refundAddress: payable(msg.sender),
+		refundAddress: refundAddress,
	        zroPaymentAddress: address(0x0),
	        adapterParams: adapterParams
	    });

Assessed type

ETH-Transfer

Summary

Low Risk

[L-01] Refund calls can fail and funds will be locked inside UTBExecutor

When a target call fails, a refund should be issued to the user's refund address.

However, it's not guaranteed that the refund call will succeed: in this case, these funds will be permanently locked inside UTBExecutor:

	if (token == address(0)) {
	    (success, ) = target.call{value: amount}(payload);
	    if (!success) {
@>	        (refund.call{value: amount}(""));
	    }
	    return;
	}

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTBExecutor.sol#L51-L55

[L-02] Anyone can swap freely without paying fees when signer is not set

Fees are collected each time a user performs a swap, which are collected through collectFees:

	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"); //@audit L - no check for recovered == 0?
	    if (fees.feeToken != address(0)) {
	        IERC20(fees.feeToken).transferFrom(
	            utb,
	            address(this),
	            fees.feeAmount
	        );
	    }
	}

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTBFeeCollector.sol#L53-L54

signer is initially set to the zero address, so the expected behavior seems to be that every transaction should revert until the signer address is set to a valid address.

However, users can pass a malformed signature so that ecrecover returns zero: in this case the require will pass, and users will not pay any fees.

[L-03] Deadline not set when swapping

The deadline is not set when swapping, as it's commented out on ExactOutputParams, and it's missing on ExactInputParams.

	IV3SwapRouter.ExactOutputParams memory params = IV3SwapRouter
	    .ExactOutputParams({
	        path: swapParams.path,
	        recipient: address(this), //@audit L - deadline not set
	        //deadline: block.timestamp,
	        amountOut: swapParams.amountOut,
	        amountInMaximum: swapParams.amountIn
	    });

https://github.com/code-423n4/2024-01-decent/blob/main/src/swappers/UniSwapper.sol#L153

	IV3SwapRouter.ExactInputParams memory params = IV3SwapRouter
	    .ExactInputParams({
	        path: swapParams.path,
	        recipient: address(this),
	        amountIn: swapParams.amountIn,
	        amountOutMinimum: swapParams.amountOut
	    });

https://github.com/code-423n4/2024-01-decent/blob/main/src/swappers/UniSwapper.sol#L129-L135

This will cause unfavorable swaps to the users, especially if the TX stays on the mempool for a long time.

It's recommended to add a deadline parameter, preferably as a swapParams field, so the user can set it.

[L-04] Excess value is not refunded to the user when swapping native

When the tokenIn is native, the msg.value needs to be greater or equal than swapParams.amountIn:

	if (swapParams.tokenIn == address(0)) {
@>	    require(msg.value >= swapParams.amountIn, "not enough native"); //@audit L - msg.value excess not refunded
	    wrapped.deposit{value: swapParams.amountIn}();
	    swapParams.tokenIn = address(wrapped);
	    swapInstructions.swapPayload = swapper.updateSwapParams(
	        swapParams,
	        swapInstructions.swapPayload
	    );
	}

However, when msg.value is greater, the excess is not refunded to the user.

Consider either changing the require so that the msg.value is strictly equal to the amountIn, or as alternative, refund the excess amount.

[L-05] onlyIfWeHaveEnoughReserves fails if there are enough reserves

Due to an off-by-one error, this modifier will fail even when the balance is sufficient.

The balance needs to be amount + 1 wei to avoid a revert. This can cause some issues when the balance is zero, and a precise amount is sent to a contract, if the caller doesn't remember to top it with 1 additional wei:

modifier onlyIfWeHaveEnoughReserves(uint256 amount) {
    require(weth.balanceOf(address(this)) > amount, "not enough reserves"); //@audit L - fails with enough reserves
    _;
}

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

Consider modifying the require so that it doesn't fail when weth.balanceOf(address(this)) >= amount instead.

[L-06] Funds can be locked when a user adds weth liquidity with some msg.value

A user can call by mistake addLiquidityWeth with some msg.value: these funds will not be used and they will be locked inside the contract

function addLiquidityWeth(
        uint256 amount
) public payable userDepositing(amount) {
    weth.transferFrom(msg.sender, address(this), amount);
    dcntEth.mint(address(this), amount);
}

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

Consider removing payable from the function signature.

[L-07] Approvals are not canceled after a partial transfer

It's not guaranteed that the target will consume the entire allowance after a call:

	uint256 balanceBefore = weth.balanceOf(address(this));
	weth.approve(target, amount); 

    (bool success, ) = target.call(callPayload); 
    //@audit L - approval not canceled after the call

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

Consider adding a weth.approve(target, 0); after the actual call to avoid any leftover approvals.