Decent - Soliditors's results

Lines of code

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

Vulnerability details

Description

The DcntEth OFTV2 contract, used in the Decent bridging system, implements a setRouter public function, which only purpose is to set the value of the router variable. This variable is used in the onlyRouter modifier, applied to mint and burn functions, to make sure only the Decent router is allowed to mint and burn DcntEth tokens.

The problem is that setRouter function doesn't include any access control modifier.

Impact

The impact of the issue is high as it results in a potential DOS of the whole Decent bridging system.

• Likelihood is high as anyone can simply call setRouter public function, passing any random address as parameter.
• Cost is low, as it only requires to call a function with low gas consumption

Consequences are really bad for the system, with either:

• Temporary of permanent DOS of Decent bridge. Indeed, the attacker could watch the mempool and call gain setRoute each time the team tries to temporary resolve the DOS
• Minting or burning any amount of tokens by the attacker through mint and burn functions, if the attacker passes his own address when calling setRouter function.

Proof of Concept

Simply call the following function, using either the address 0 or your own address.

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

As both mint and burn functions uses the OnlyRouter modifier:

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

    function burn(address _from, uint256 _amount) public onlyRouter {
        _burn(_from, _amount);
    }

The result will be either:

• systematic revert when these functions are called by the DecentEthRouter in addLiquidityEth, addLiquidityWeth, removeLiquidityEth, and removeLiquidityWeth functions

• ability to mint any amount of tokens with mint function, or to burn any amount of token which is less or equal to the total supply with burn function

Tools Used

Manual review

Recommended Mitigation Steps

We suggest to protect the setRouter function with the onlyOwner modifier. It could also be a good idea to protect it with some OnlyOnce modifier if Decent protocol wants to make its system even more credible neutral.

Assessed type

Access Control

Lines of code

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

Vulnerability details

When delivering tokens cross-chain, DecentBridgeExecutor handles the extra tokens provided by transferring them to the from address:

File: DecentBridgeExecutor.sol
24:     function _executeWeth(
---
29:     ) private {
---
35:         if (!success) {
36:             weth.transfer(from, amount);
37:             return;
38:         }
---
43:         // refund the sender with excess WETH
44:         weth.transfer(from, remainingAfterCall);
---
54:     function _executeEth(
---
59:     ) private {
60:         weth.withdraw(amount);
61:         (bool success, ) = target.call{value: amount}(callPayload);
62:         if (!success) {
63:             payable(from).transfer(amount);
64:         }
65:     }

On L36, L44, L63, we can see that from is used for refunding token left over from a failed call to target, or in case target did not pick up the approved tokens.

If we work backwards through the code, we can see that from is passed by the caller of the execute function:

File: DecentBridgeExecutor.sol
68:     function execute(
69:         address from,
---
74:     ) public onlyOwner {
---
77:         if (!gasCurrencyIsEth || !deliverEth) {
78:             _executeWeth(from, target, amount, callPayload);
79:         } else {
80:             _executeEth(from, target, amount, callPayload);
81:         }
82:     }

The execute function is in turn called by DecentEthRouter.onOFTReceived, that extracts it from the payload it receives:

File: DecentEthRouter.sol
237:     function onOFTReceived(
---
243:         bytes memory _payload
---
244:     ) external override onlyLzApp {
---
245:         (uint8 msgType, address _from, address _to, bool deliverEth) = abi
246:             .decode(_payload, (uint8, address, address, bool));
---
254:             );
---
278:         } else {
279:             weth.approve(address(executor), _amount);
280:             executor.execute(_from, _to, deliverEth, _amount, callPayload);
281:         }
282:     }

This payload is encoded by the DecentEthRouter on the source chain, and _from (the second positional argument) is hardcoded from msg.sender:

File: DecentEthRouter.sol
103:             payload = abi.encode(
104:                 msgType,
105:                 msg.sender,
106:                 _toAddress,
107:                 deliverEth,
108:                 additionalPayload
109:             );

This means that whatever address calls the DecentEthRouter.bridgeWithPayload function on the source chain, will be receive token refunds on the destination chain.

This creates a problem because this msg.sender, on the destination chain, may be an unused address or belong to a different entity.

Hacks involving funds routed to an address that exists only on the source chain are known to the community.

Impact

Tokens refunded by DecentBridgeExecutor may end up being permanently locked, or stolen by a user different from the intended one - the sender.

Proof of Concept

This issue can be easily reproduced as follows:

• instruct a DecentEthRouter.bridgeWithPayload call to a contract address with an invalid calldata
• check the address where DecentBridgeExecutor routed the unused funds

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider adding to the payload a refundee, and enforcing this value to be set if the DecentEthRouter.bridgeWithPayload caller is a contract.

This may also cover networks that calculate contract addresses differently - like zkSync era.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Description

DecentEthRouter contract implements a bridge function and a bridgeWithPayload function, which are both public and payable, without access control.

bridgeWithPayload function is called by the DecentBridgeAdapter contract in bridge function, in order to execute the funds bridging requested by the user in UTB contract through bridgeAndExecute function.

The issue arises because anyone is able to interact directly with DecentEthRouter contract to perform a bridge operation, by-passing the fees due to Decent protocol. Indeed, these fees are taken from the user in UTB contract thanks to retrieveAndCollectFees modifier.

Impact

The impact of this issue can be considered as high as it results in unpaid fees to the protocol by users. Anyone can use bridging services proposed by Decent without paying fees to the protocol.

Proof of Concept

Simply call bridge or bridgeWithPayload function in DecentEthRouter contract, to perform a token bridge without paying fees to Decent.

    function bridge(
        uint16 _dstChainId,
        address _toAddress,
        uint256 _amount,
        uint64 _dstGasForCall,
        bool deliverEth // if false, delivers WETH
    ) public payable {
        _bridgeWithPayload(MT_ETH_TRANSFER, _dstChainId, _toAddress, _amount, _dstGasForCall, bytes(""), deliverEth);
    }

    function bridgeWithPayload(
        uint16 _dstChainId,
        address _toAddress,
        uint256 _amount,
        bool deliverEth,
        uint64 _dstGasForCall,
        bytes memory additionalPayload
    ) public payable {
        return _bridgeWithPayload(
            MT_ETH_TRANSFER_WITH_PAYLOAD,
            _dstChainId,
            _toAddress,
            _amount,
            _dstGasForCall,
            additionalPayload,
            deliverEth
        );
    }

Tools Used

Manual review

Recommended Mitigation Steps

We suggest to:

• remove bridge function, which is never called by Decent adapter contract, and is therefore useless
• add a modifier to bridgeWithPayload function, to make sure this function is only callable by DecentBridgeAdapter

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L311

Vulnerability details

Description

UTB contract is the main entry point for users to interact with Decent protocol. Two functions are supposed to be accessible to them:

• swapAndExecute function, which swaps currency from the incoming to the outgoing token and executes a transaction with payment.
• bridgeAndExecute function, which bridges funds in native or ERC20 and a payment transaction payload to the destination chain

Both functions use the retrieveAndCollectFees modifier, which transfers due fees from the user to UTB contract, and finally to UTBFeeCollector contract.

On the other hand, receiveFromBridge function is called by BridgeAdapter contracts, i.e. DecentBridgeAdapter and StargateBridgeAdapter. Its purpose is to receive funds from bridge adapters, execute a swap and a payment transaction.

The issue here is that there is no access control for this public function. Hence, anyone can directly call receiveFromBridge function instead of calling swapAndExecute function. Please note that the function is not payable. Hence, it will not work if the token the be swapped is native eth, but it will work for any ERC20 token.

Impact

The impact of this issue is high as it results in unpaid fees due to the protocol by the users, leading to loss of funds for the protocol.

Proof of Concept

swapAndExecute and receiveFromBridge functions basically do the exact same thing: calling _swapAndExecute private function.

    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 receiveFromBridge(
        SwapInstructions memory postBridge,
        address target,
        address paymentOperator,
        bytes memory payload,
        address payable refund
    ) public {
        _swapAndExecute(postBridge, target, paymentOperator, payload, refund);
    }

The only difference is that swapAndExecute applies the retrieveAndCollectFees modifier.

Simply call receiveFromBridge function with desired parameters to do your swap and execute the transaction, without paying fees.

Tools Used

Manual review

Recommended Mitigation Steps

We suggest to protect the receiveFromBridge with a dedicated onlyBridgeAdapter modifier. This modifier should only allow authorised Bridge Adapters to call the function.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/bridge_adapters/StargateBridgeAdapter.sol#L176

Vulnerability details

Description

The amount to be used in the bridge is calculated as follows:

    function getBridgedAmount(uint256 amt2Bridge, address, /*tokenIn*/ address /*tokenOut*/ )
        external
        pure
        returns (uint256)
    {
        return (amt2Bridge * (1e4 - SG_FEE_BPS)) / 1e4;
    }

callBridge function uses this exact same computation to pass the min amount the user accepts to receive from the bridge on the destination chain:

            (amt2Bridge * (10000 - SG_FEE_BPS)) / 10000, // the min qty you would accept on the destination, fee is 6 bips

callBridge function then calls Stargate router swap function, which calls swap function on the Stargate bridge contract.

The issue arises because the bridge contract checks that the minAmount to be received is less or equal than what is really going to be received:

        require(s.amount.add(s.eqReward) >= minAmountSD, "Stargate: slippage too high");

Currently, Stargate allows a max fee of 0.06%, depending on the demand. This means that if Stargate DAO decides to modify the max fees, the Decent bridge adapter might not work as exepected with a risk of DOS, as this fee is hardcoded to 0.06%

Impact

The impact of this issue can be considered as medium as it results in a DOS of the Decent bridging system, only if the DAO increases its max fee.

Proof of Concept

The minimum amount a user accepts to receive after the bridge operation is hardcoded to 0.06% of the amount to bridge:

            (amt2Bridge * (10000 - SG_FEE_BPS)) / 10000, // the min qty you would accept on the destination

Now, suppose that Stargate increases its max fees to 0.1%. If the max fee is applied to the amount to bridge, then, the following check will fail:

        require(s.amount.add(s.eqReward) >= minAmountSD, "Stargate: slippage too high");

Indeed, minAmountSD will be greater than the real amount to be received.

Tools Used

Manual review

Recommended Mitigation Steps

We suggest to make sure that the SG_FEE_BPS is not hardcoded to 0.06%, for example with a setter function able to modify its value, protected by the onlyOwner modifier.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/bridge_adapters/StargateBridgeAdapter.sol#L207-L207

Vulnerability details

Description

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

Assessed type

DoS