Decent - Aamir's results

Lines of code

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

Vulnerability details

The DcntEth::setRouter(...) function lacks proper access control, thereby granting unrestricted access to critical functions for any user.

Impact

The DcntEth::setRouter(...) function is instrumental in setting the router within the DcntEth contract. This router, once set, can invoke essential functions such as _mint(...) and _burn(...). The absence of access control on DcntEth::setRouter(...) allows potential attackers to set a malicious contract as a router, enabling them to illicitly obtain tokens by invoking these crucial functions.

Proof of Concept

The lack of access control is evident in the following public function, where anyone can modify the router:

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

GitHub: 20-22

Here is a test for PoC:

Add the below given test in DecentWethRouterNoFork.t.sol

    function test_AnyoneCanSetTheNewRouter() public {
        address newRouter = address(0xdeadbeef);
        vm.prank(makeAddr("alice"));
        dcntEth.setRouter(newRouter);
        assertEq(dcntEth.router(), newRouter);
    }

Output:

AAMIR@Victus MINGW64 /d/decent-2nd-part-audit (main)
$ forge test --mt test_AnyoneCanSetTheNewRouter -vv
[⠢] Compiling...
[⠔] Compiling 1 files with 0.8.20
[⠒] Solc 0.8.20 finished in 3.51s
Compiler run successful!

Running 1 test for test/DecentWethRouterNoFork.t.sol:DecentEthRouterNonEthChainTest
[PASS] test_AnyoneCanSetTheNewRouter() (gas: 16242)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.64ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review

Recommended Mitigation Steps

It is strongly advised to implement proper access controls to restrict unauthorized access to the setRouter function.

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

Assessed type

Access Control

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#L101

Vulnerability details

If a contract is trying to bridge tokens using DecentEthRouter::bridgeWithPayload(...), it might loose tokens if the call reverted on the destination chain or some tokens are refunded back.

Impact

The function DecentEthRouter::bridgeWithPayload(...) facilitates token bridging between chains and executes a call with the sent tokens. The calldata for the destination chain is constructed using DecentEthRouter::_getCallParams(...). The calldata for the call is created like this:

File: DecentBridgeExecutor.sol

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

        if (msgType == MT_ETH_TRANSFER) {
            payload = abi.encode(msgType, msg.sender, _toAddress, deliverEth);
        } else {
 @>           payload = abi.encode(
                msgType,
                msg.sender,
                _toAddress,
                deliverEth,
                additionalPayload
            );
        }
    }

GitHub: 102-111

The payload consists of the following components:

1. msgType: The type of the message we want to send.
2. from: The address of the sender. (I.e. msg.sender in the above given code)
3. to: The address of receiver. (i.e. toAddress)
4. deliverEth: Whether the tokens delivered should be in ETH or not.
5. additionalPayload: Payload for the call on the destination chain.

Notably, the from argument is set as the address of the msg.sender.

Execution Flow on destination chain

1. The calldata is received on the destination router, triggering DecentEthRouter::onOFTReceived(...) and then it is decoded.
2. The decoded arguments are sent to DecentEthExecutor::execute(...).
3. A call is made to the to address, and if it fails, the received tokens are sent back to the from address on the destination chain. Also if not all tokens are spent on the external call, then remaining tokens will be sent back to from address.

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

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

    function _executeEth(
        address from,
        address target,
        uint256 amount,
        bytes memory callPayload
    ) private {
        weth.withdraw(amount);
        (bool success, ) = target.call{value: amount}(callPayload);
        if (!success) {
            // @audit funds will lost in case of gnosis safe wallet
@>            payable(from).transfer(amount);
        }
    }

GitHub: [63]

Vulnerability

If the sender of DecentEthRouter::bridgeWithPayload(...) is a contract, then it is not necessary if the same address exist on the destination chain. If not, the from argument will point out to an arbitrary address on the destination chain and sent tokens will be lost. The token loss can happen in two way:

1. If the external call fails, the tokens will be sent to from address.
2. If all of the tokens are not spent on the external call then the remaining balance will be refunded to the from address.

Also if a Multi-sig is used to send the call then it is not necessary if the same Multi-sig is owned by the same owner on destination chain. If not, either the Multi-sig will be owned by some other address or not yet created. In that case an attacker can just create multiple Multi-sig's wallets until he gets the same Multi-sig wallet address and can steal the tokens.

As explained in detail on this article written by Rekt, it is possible to gain control of the same address for contract accounts in a different chain; especially for those contract accounts that are deployed using the Gnosis Safe contracts.

Proof of Concept

Add the below given test in DecentEthRouterWeth2Weth.t.sol

    function testSenderWillLooseTokensIfItisContractOrMultiSigAndTheCallFailsOnDestination()
        public
    {
        uint256 bridgeAmount = 1 ether;

        // deploying a contract on src chain that will send the tokens
        switchTo(srcChain);
        SenderContract senderContract = new SenderContract();

        // checking the owner
        assertEq(senderContract.owner(), address(this));
        
        // creating a contract on target chain that will receive the tokens
        switchTo(dstChain);
        FailTarget target = new FailTarget();

        // giving some ether to the sender 
        dealTo(srcChain, address(senderContract), bridgeAmount);
        mintWethTo(srcChain, address(senderContract), bridgeAmount);

        // checking the balance of the sender contract
        assertEq(address(senderContract).balance, bridgeAmount);

        CoolCat coolCat = birthCoolCat();

        // Starting the briding process
        startRecordingLzMessages();

        BridgeParams memory params = BridgeParams({
            src: srcChain,
            dst: dstChain,
            fromAddress: address(senderContract),
            toAddress: address(target),
            amount: bridgeAmount
        });

        uint fees = attemptBridge(
            params,
            MT_ETH_TRANSFER_WITH_PAYLOAD,
            false,
            GAS_FOR_MEOW_MEOW,
            abi.encodeCall(FailTarget.fail,())
        );

        deliverLzMessageAtDestination(srcChain, dstChain, GAS_FOR_MEOW_MEOW);

        // since the target will fail, the weth will be refunded to the address(senderContract)
        assertWethBalanceEq(dstChain, address(target), 0);
        
        // address(senderContract) will receive the tokens on destination chain which doesn't exist
        assertWethBalanceEq(dstChain, address(senderContract), bridgeAmount);
    }

Import the following contract:

import {BridgeParams, RouterActions} from "./common/RouterActions.sol";

Also add the following contracts in the file:

contract SenderContract{
    // Function to receive Ether. msg.data must be empty
    address public owner;

    constructor(){
        owner = msg.sender;
    }
    function call(bytes memory data) public payable returns (bool, bytes memory) {
        (bool success, bytes memory returndata) = address(this).call{value: msg.value}(data);
        return (success, returndata);
    }
    receive() external payable {}

    // Fallback function is called when msg.data is not empty
    fallback() external payable {}
}

contract FailTarget{
    function fail() public {
        revert("fail");
    }
}

Output:

   │   │   │   ├─ emit CallOFTReceivedSuccess(_srcChainId: 106, _srcAddress: 0xf62849f9a0b5bf2913b396098f7c7019b51a820aa0cb889707d426a7a386870a03bc70d1b0697598, _nonce: 1, _hash: 0x73f971836d979efb46fdb746c7fbb4835a41cb04d668f2f3a30ba0eed7e1f51e)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [0] VM::selectFork(6)
    │   └─ ← ()
    ├─ [2608] polygon_WETH::balanceOf(FailTarget: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::selectFork(6)
    │   └─ ← ()
    ├─ [608] polygon_WETH::balanceOf(SenderContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211]) [staticcall]
    │   └─ ← 1000000000000000000 [1e18]
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 52.36s
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review

Recommended Mitigation Steps

Is is recommended to add another argument in the DecentEthRouter::bridgeWithPayload(...) to represent the refund address on the destination chain and add it to payload. Then send refund the tokens to this address instead.

Assessed type

Other

Lines of code

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

Vulnerability details

DecentEthRouter::onOFTReceived(...) function interpret any type of message call as a transfer token call if the Weth balance of the contract becomes less than the bridged amount. This will call loss of the tokens to the user.

Impact

DecentEthRouter::onOFTReceived(...) will be called on Destination chain whenever tokens are bridged. This function is responsible for transferring the tokens to the receiver on destination chain and if there is a calldata then the same function will be used to divert the call to DecentBridgeExecutor along with the received token and calldata so that a call to the external contract can be made. This calldata transferring feature enables cross-chain transactions using bridged tokens.

But there is a flaw in this line of the function:

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

        emit ReceivedDecentEth(
            msgType,
            _srcChainId,
            _from,
            _to,
            _amount,
            callPayload
        );

@>        if (weth.balanceOf(address(this)) < _amount) {
            dcntEth.transfer(_to, _amount);
            return;
        }

        ...

GitHub: 266-269

This line checks that if the WETH balance of the router contract is less than the bridged _amount then the dcntEth will be transferred to the receiver (in this case _to) address instead of WETH and function call execution will stop there.

This is not a problem if the bridged token is meant to be transferred to the receiver address. But the problem will occur when the token is bridged in order to make a call to an external contract for e.g minting an NFT. In that case the tokens will be transferred to the receiver contract that was meant to be transferred during the external call. Now the call will also not happen and user will be in loss of tokens.

Let's take an example to understand it better:

1. Alice wants to buy an NFT on polygon that is worth 15 ether. She has 15 ether on Avalanche. So She decides to bridge the token using the DecentEthRouter contract along with the calldata for the NFT minting.
2. Let's say destination DecentEthRouter has only 10 WETH and 15 DcntETH.
3. Alice calls the DecentEthRouter::bridge(...) function along with calldata.
4. On destination, DecentEthRouter::onOFTReceived(...) would be called. But since the contract has only 10 WETH, the above given condition will met and 15 dcntEth will be transferred to the NFT contract. But No call for minting the NFT will be done. The bridging call is marked success and bridging is finished.

Because of this Alice has lost her tokens as well as no NFT is minted to her.

Proof of Concept

Here is a test for the proof. This test exactly represent the example given above. Add this test in DecentEthRouterWeth2Weth.t.sol

    function testIfDestinationRouterHadLessWethThenTheBridingingAmountForCallWillBeInterpretedAsTransferCall()
        public
    {
        uint256 nftPrice = 15 ether;
        
        // giving some ether to the sender
        mintWrappedTo(srcChain, address(this), nftPrice);

        // deploying NFT on dst chain
        switchTo(dstChain);
        NFT nft = new NFT();


        // getting the weth balance of the dcnt eth router on dst chain
        // will have 100 weth
        IWETH dstWeth = IWETH(wethLookup[dstChain]);
        uint256 wethBalanceRouterDstChain = dstWeth.balanceOf(address(routerLookup[dstChain]));

        // getting the dcnt eth router balance on dst chain
        // will have 100 dcnt eth
        DcntEth dcntETHDstChain = DcntEth(dcntEthLookup[dstChain]);
        uint256 dcntEthBalanceRouterDstChain = dcntETHDstChain.balanceOf(address(routerLookup[dstChain]));

        // burning some dcntETh and transferring weth from the router so that it will have less balance
        // we need 10 weth and 15 dcnt eth balance on the router
        vm.prank(dcntETHDstChain.owner());
        dcntETHDstChain.burnByOwner(address(routerLookup[dstChain]), 85 ether);

        vm.startPrank(address(routerLookup[dstChain]));
        dstWeth.transfer(address(0xdead), 90 ether);
        vm.stopPrank();

        // getting the new balance of the dcnt eth router on dst chain
        uint256 dcntEthBalanceRouterDstChainAfterMint = dcntETHDstChain.balanceOf(address(routerLookup[dstChain]));

        // will be 15 dcnt
        assertEq(dcntEthBalanceRouterDstChainAfterMint, dcntEthBalanceRouterDstChain - 85 ether);
        assertEq(dcntEthBalanceRouterDstChainAfterMint, 15 ether);

        // weth balance of the reouter on dst chain
        uint256 wethBalanceRouterDstChainAfterMint = dstWeth.balanceOf(address(routerLookup[dstChain]));

        // will be 10 weth
        assertEq(wethBalanceRouterDstChainAfterMint, wethBalanceRouterDstChain - 90 ether);
        assertEq(wethBalanceRouterDstChainAfterMint, 10 ether);
        
        // minting the NFT from srchain to dst chain
        // Starting the briding process
        startRecordingLzMessages();

        BridgeParams memory params = BridgeParams({
            src: srcChain,
            dst: dstChain,
            fromAddress: address(this),
            toAddress: address(nft),
            amount: nftPrice
        });

        uint fees = attemptBridge(
            params,
            MT_ETH_TRANSFER_WITH_PAYLOAD,
            false,
            GAS_FOR_MEOW_MEOW,
            abi.encodeCall(NFT.mint,(address(this), 1))
        );

        deliverLzMessageAtDestination(srcChain, dstChain, GAS_FOR_MEOW_MEOW);

        switchTo(dstChain);
        // getting the owner of the NFT. But this will revert as the NFT was never minted
        vm.expectRevert();
        address owner = nft.ownerOf(1);

        // But NFT contract should have received the dcnt ETH corresponding to the price of the NFT
        // getting the dcnt eth balance of the nft contract
        uint256 dcntEthBalanceNFT = dcntETHDstChain.balanceOf(address(nft));
        require(dcntEthBalanceNFT == nftPrice, "dcntEthBalanceNFT != nftPrice");
    }

Also import the following contracts:

import {BridgeParams, RouterActions} from "./common/RouterActions.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {IWETH} from "src/interfaces/IWETH.sol";
import {DcntEth} from "src/DcntEth.sol";

Add this NFT contract to the file as well:

contract NFT is ERC721 {
    constructor() ERC721("NFT", "NFT") {}

    // paid mint. Only 15 ether or more
    function mint(address to, uint256 tokenId) public payable {
        if(msg.value < 15 ether){
            revert("not enough ether");
        }
        _mint(to, tokenId);
    }
}

Output:

[PASS] testIfDestinationRouterHadLessWethThenTheBridingingAmountForCallWillBeInterpretedAsTransferCall() (gas: 1662986)
Logs:
  dcntEth & router:  0xF62849F9A0B5Bf2913b396098F7c7019b51A820a 0x2e234DAe75C793f67A35089C9d99245E1C58470b
  dcntEth & router:  0xa0Cb889707d426A7A386870A03bc70d1b0697598 0xc7183455a4C133Ae270771860664b6B7ec320bB1
  impersonating as 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  impersonating as 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  impersonating as 0x5E12fc70B97902AC19B9cB87F2aC5a8593769779
  impersonating as 0x5E12fc70B97902AC19B9cB87F2aC5a8593769779
  impersonating as 0x1eED63EfBA5f81D95bfe37d82C8E736b974F477b
  impersonating as 0x1eED63EfBA5f81D95bfe37d82C8E736b974F477b
  impersonating as 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  impersonating as 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  Provided Gas Limit:  600000
  0xe9bded5f24a4168e4f3bf44e00298c993b22376aad8c58c7dda9718a54cbea82
  0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000001a50000000000000001006af62849f9a0b5bf2913b396098f7c7019b51a820a006da0cb889707d426a7a386870a03bc70d1b069759801000000000000000000000000c7183455a4c133ae270771860664b6b7ec320bb1d02ab486cedc00000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b000000000007a12000000000000000000000000000000000000000000000000000000000000000010000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff211000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000004440c10f190000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
  impersonating as 0x4D73AdB72bC3DD368966edD0f0b2148401A178E2

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 42.72s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to do the following:

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

        emit ReceivedDecentEth(
            msgType,
            _srcChainId,
            _from,
            _to,
            _amount,
            callPayload
        );

        if (weth.balanceOf(address(this)) < _amount) {
+           if(msgType == MT_ETH_TRANSFER)
                dcntEth.transfer(_to, _amount);
+           else
+               dcntEth.transfer(_from, _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);
        }
    }

Assessed type

Error

Summary

Lows

L-0 Signature Can be replayed and a user can get away by paying less fee <a id="low-0"></a>

UTBFeeCollector::collectFees(...) uses ECDSA signatures in order to verify if the correct fee is paid by the user. And as per the sponsors the fee can be paid as a BPS of the amount swapped or bridged or in any other way and it is not fixed. So there are chances that it might be raised in the future. A user can exploit this behaviour. He can use an already used signature that had lower fees and can get away by paying the lower fee than current fee. All he has to do is use the same paramaters for the new call that he or someone else used before. This is exploitable because the signatures are not completely unique. It is a combination of the following things:

BANNER + SwapAndExecuteInstructions + FeeStructure

where,

BANNER: "\x19Ethereum Signed Message:\n32"

SwapAndExecuteInstructions:

struct SwapAndExecuteInstructions {
    SwapInstructions swapInstructions;
    address target;
    address paymentOperator;
    address payable refund;
    bytes payload;
}

FeeStructure:

struct FeeStructure {
    uint256 bridgeFee;
    address feeToken;
    uint256 feeAmount;
}

The reason why the signature is not unique is because these all are user supplied inputs and if a user wants to make the same call in the future(with same parameter) all he has to do is pick up the old signature and use it. And it will still work. But this type of situation will only work if the following condition is met:

1. The fee BPS has increasd.
2. If it is not an external call that itself will be unique like, minting an NFT.

Not only that a user can pick up the signature from other chain that has same parameter with low fee and it will still work.

Mitigation

It is recommended to make the signature unique. Add something like an incremental nonce and chainId to the signature.

---

L-1 All Approvals should be cleared at the end in DecentEthExector::_executeWeth() <a id="low-1"></a>

If the call fails in DecentEthExector::_executeWeth(), then the weth is transferred to the target address but the approval to from still exists. This can be exploited by the someone if the contract is under an attack or a tokens is used that will only allow approval if the old approval is not zero(in future ofcourse). Also there will be an active approval if all of tokens are not used by the target contract.

    function _executeWeth(
        address from,
        address target,
        uint256 amount,
        bytes memory callPayload
    ) private {
        uint256 balanceBefore = weth.balanceOf(address(this));
        // @audit approval will remain
@>        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);
    }

Mitigation

Clear any approvals at the end of the functions.

---

L-2 Add functionality to retryfailed Message from Layerzero

If message is failed to be sent by the layerzero due to some reason, there is a functionality that can be used to retry the failed message. It can be called by anyone. But in order to do that someone has to go to the layerzero endpoint and pass in the correct parameters in lzReceive to retreive the Message. But there is no function in DecentEthRouter or any other contract to do that. This functionality should be added so that user does not have to go anywhere.

To learn more about it, go to this link

---

L-3 Contracts that holds tokens should have emergency withdraw tokens functions <a id="low-3"></a>

Contracts that holds ETH or ERC20 should have emergencey withdraw tokens function. Atleast DecentEthRouter should have it. So that if the tokens are stucked in the contract due to any reason then it can be retreived from the contract. Or if any unexpected token is sent than it can be recoverd as well. These functions are also helpful in case the contract is under attack. But there is no such function. It is recommended to add these functions.

---

L-4 There should be deadline for swap token calls <a id="low-4"></a>

Currently swap functions doesn't accept deadline parameter for swaps. This deadline ensures that the tokens are received within a fixed time period so that no unexpeted results comes from the swaps. But there is no such parameter used. It is recommeded to add the deadline as well