Decent - peanuts's results

Lines of code

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

Vulnerability details

Impact

Anyone can mint any amount of DcntEth and also drain the router contract of WETH.

Proof of Concept

This is the DcntEth.setRouter function:

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


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

The setRouter() function is public and has no restricted assess, so anyone can change the router variable. This router variable is used as a modifier to mint and burn DcntEth token.

    modifier onlyRouter() {
        require(msg.sender == router);
        _;
    }

Anyone can set their own address as the router and mint any amount of tokens. They can also burn other people tokens.

With the DcntEth tokens they minted, they can drain the DecentEthRouter contract of its WETH tokens (if it has any).

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


    /// @inheritdoc IDecentEthRouter
    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

Use the onlyOwner modifier in the setRouter() function. Make sure the router variable can only be changed by the owner.

Assessed type

Invalid Validation

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L197-L215 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L259-L274

Vulnerability details

Impact

Protocol will not get bridge fees.

Proof of Concept

In UTB.sol, the entry point for bridging is through bridgeAndExecute(). Users will fill in the BridgeInstructions, FeeStructure and signature as the input parameter. There are three functions called, retrieveAndCollectFees(), swapAndModifyPostBridge(), callBridge(). Note that FeeStructure is a user input.

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

Focusing on callBridge, note that the bridgeFee is calculated because calling bridge() on the BridgeAdapter.

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

This bridge fee is from the user input of bridgeAndExecute(), which means the user can set the bridge fee to zero. It wouldn't work on the Stargate bridge because stargate calculates the fees directly in the router -> pool itself.

  // request fee params from library
        SwapObj memory s = IStargateFeeLibrary(feeLibrary).getFees(poolId, _dstPoolId, _dstChainId, _from, amountSD);

https://optimistic.etherscan.io/address/0xdecc0c09c3b5f6e92ef4184125d5648a66e35298#code

However, when DecentBridgeAdapter.bridge() is called, it will eventually call router.bridgeWithPayload.

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

The DecentEthRouter.sol does not calculate any fees in the router.

Thus, users can call bridge() on DecentEthRouter.sol directly to skip payment of protocol fees and bridge fees since it has public visibility without any modifiers.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend calculating the fees inside the router contract, like how RadiantOFT does it (before the _send() function).

Radiant line 105 (_send() function), line 115-116 (fees). https://vscode.blockscan.com/arbitrum-one/0x41e018EB5c52d5A400fFb891B8569C8AcFe905f1

So, even if users call the DecantETHRouter directly, they will still have to pay a bridge fee.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

receiveFromBridge() can be called by anyone. Since most of the parameters are user inputs, one impact is the user can call _swapAndExecute() without having to pay the UTB fees since there is no retrieveAndCollectFees modifier.

Another impact is that since the adapter holds the funds after bridging, users can simply call receiveFromBridge() and conduct an arbitrary function execution to steal funds from the adapter.

Proof of Concept

There is a receiveFromBridge() function in UTB.sol, which supposedly receives funds from the bridge adapter, executes a swap, and executes a payment transaction.

It has public visibility with no added modifiers.

    /**
>    * @dev Receives funds from the bridge adapter, executes a swap, and executes a payment transaction.
     * @param postBridge The swapper ID and calldata to execute a swap.
     * @param target The address of the target contract for the payment transaction.
     * @param paymentOperator The operator address for payment transfers requiring ERC20 approvals.
     * @param payload The calldata to execute the payment transaction.
     * @param refund The account receiving any refunds, typically the EOA which initiated the transaction.
     */
    function receiveFromBridge(
        SwapInstructions memory postBridge,
        address target,
        address paymentOperator,
        bytes memory payload,
        address payable refund
>   ) public {
>       _swapAndExecute(postBridge, target, paymentOperator, payload, refund);
    }

This function calls _swapAndExecute(), which performs a swap through a swapper and performs and arbitrary execution.

This function should be called only by the adapter, otherwise users can just call this function in place of the swapAndExecute() function and skip payment of fees.

    //@audit -- This function also calls _swapAndExecute, with the same parameters, but there is an added retrieveAndCollectFees parameter.
    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
        );
    }

Also, the destination chain adapter will hold the funds. Users can simply call receiveFromBridge() with an arbitrary function call (in the payload function) and use other people's funds.

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

Tools Used

Recommended Mitigation Steps

Add an adapter modifier so that only the adapter (currently just stargate and decent) can call the UTB.receiveFromBridge() function.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The slippage value is fixed and the range is too small, resulting in function unlikely to succeed.

Proof of Concept

In StargateBridgeAdapter.callBridge(), there is a parameter in router.swap that is meant for slippage.

    function callBridge(
        uint256 amt2Bridge,
        uint256 dstChainId,
        bytes memory bridgePayload,
        bytes calldata additionalArgs,
        address payable refund
    ) private {
        router.swap{value: msg.value}(
            getDstChainId(additionalArgs), //lzBridgeData._dstChainId, // send to LayerZero chainId
            getSrcPoolId(additionalArgs), //lzBridgeData._srcPoolId, // source pool id
            getDstPoolId(additionalArgs), //lzBridgeData._dstPoolId, // dst pool id
            refund, // refund adddress. extra gas (if any) is returned to this address
            amt2Bridge, // quantity to swap
>           (amt2Bridge * (10000 - SG_FEE_BPS)) / 10000, // the min qty you would accept on the destination, fee is 6 bips
            getLzTxObj(additionalArgs), // additional gasLimit increase, airdrop, at address
            abi.encodePacked(getDestAdapter(dstChainId)),
            bridgePayload // bytes param, if you wish to send additional payload you can abi.encode() them here
        );
    }

This is what all the parameters are, according to the Stargate documentation:

// perform a Stargate swap() in a solidity smart contract function
// the msg.value is the "fee" that Stargate needs to pay for the cross chain message
IStargateRouter(routerAddress).swap{value:msg.value}(
    10006,                           // send to Fuji (use LayerZero chainId)
    1,                               // source pool id
    1,                               // dest pool id                 
    msg.sender,                      // refund adddress. extra gas (if any) is returned to this address
    _amountLD,                       // quantity to swap in LD, (local decimals)
>   _minAmountLD,                    // the min qty you would accept in LD (local decimals)
    IStargateRouter.lzTxObj(0, 0, "0x")  // 0 additional gasLimit increase, 0 airdrop, at 0x address
    abi.encodePacked(msg.sender),    // the address to send the tokens to on the destination
    bytes("")                        // bytes param, if you wish to send additional payload you can abi.encode() them here
);

The slippage right now is fixed at 0.06%, which is extremely low. Also, the SG_FEE_BPS is a constant variable, which cannot be changed.

If there is a need for a higher slippage, then the bridge would not work. Also, the normal slippage is around ~1% (100 BPS), as seen in a random stargate router call here:

https://phalcon.blocksec.com/explorer/tx/optimism/0xbb49cf766edb2c3f092e3fda8bd2b98da580fbf319d053468d9e11cab478ff87

// perform a Stargate swap() in a solidity smart contract function
// the msg.value is the "fee" that Stargate needs to pay for the cross chain message
IStargateRouter(routerAddress).swap{value:msg.value}(
    109,                           // send to Fuji (use LayerZero chainId)
    1,                               // source pool id
    1,                               // dest pool id                 
    (user address),                      // refund address. extra gas (if any) is returned to this address
    570516202,                       // quantity to swap in LD, (local decimals)
>   564811040,                    // the min qty you would accept in LD (local decimals)
    IStargateRouter.lzTxObj(0, 0, "0x")  // 0 additional gasLimit increase, 0 airdrop, at 0x address
    (user address),    // the address to send the tokens to on the destination
    bytes("")                        // bytes param, if you wish to send additional payload you can abi.encode() them here
);

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure the slippage is not fixed, or have the minAmount be a userInput instead.

Assessed type

MEV