UTBExecutor does not revert in case refund transfer fails, leading to tokens stuck

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTBExecutor.sol#L52-L56

Vulnerability details

Impact

• Native tokens not refunded and get stuck in UTBExecutor

Proof of Concept

If a user call function UTB.swapAndExecute then in the last step, the contract will call UTBExecutor.execute and this function is currently implemented as:

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

       ...

As you can see, if user specify tokenOut == address(0) (native tokens), then the function execute will try to call target.call, if not successful, then call refund.call{value: amount}. However, the refund call will not revert in case it fails, for example if refund is a smart contract without fallback function, causing the tokens to get stuck in UTBExecutor and not refunded to user.

The correct behavior here is to either revert if refund.call is not successful, or transfer the wrapped token instead to refund address.

Below is a POC for the above issue, save it to a file named test/UTBFeeCollectorRefundNotRevert.t.sol and run it using command:
forge test --match-path test/UTBFeeCollectorRefundNotRevert.t.sol --match-test testFeeCollectorRefundNotRevert -vvvv

You can see in the log that both function VeryCoolCat::mintWithUsdc (the target) and DummyContract::fallback(the refund) revert but the swap is still successful and native tokens get stuck in UTBExecutor.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {UTB, ISwapper, SwapInstructions, SwapAndExecuteInstructions, FeeStructure} from "../src/UTB.sol";
import {UTBExecutor} from "../src/UTBExecutor.sol";
import {UTBFeeCollector} from "../src/UTBFeeCollector.sol";
import {UniSwapper} from "../src/swappers/UniSwapper.sol";
import {SwapParams} from "../src/swappers/SwapParams.sol";
import {XChainExactOutFixture} from "./helpers/XChainExactOutFixture.sol";
import {VeryCoolCat} from "./helpers/VeryCoolCat.sol";
import {Test} from "forge-std/Test.sol";
import {IQuoterV2} from "@uniswap/v3-periphery/contracts/interfaces/IQuoterV2.sol";
import {console} from "forge-std/console.sol";
import {WETH} from "solmate/tokens/WETH.sol";
import {IV3SwapRouter} from "@uniswap/swap-contracts/interfaces/IV3SwapRouter.sol";
import {IERC20} from "forge-std/interfaces/IERC20.sol";

contract DummyContract {

}
contract MyUTBExactOutRoutesTest is Test {
	mapping(string => uint256) forkLookup;
	mapping(string => bool) gasEthLookup;
	mapping(string => address) wethLookup;
	mapping(string => address) wrappedLookup;
	mapping(string => uint256) chainIdLookup;
	mapping(string => ISwapper) swapperLookup;

	mapping(string => address) tokenLookup;

	mapping(string => address) public uniRouterLookup;

	mapping(string => IQuoterV2) uniQuoterLookup;

	uint24 constant TICK_SIZE_1 = 100;
    uint24 constant TICK_SIZE_2 = 300;
    uint24 constant TICK_SIZE_3 = 500;

    address alice = address(0xa11ce);
    address bob = address(0xb0b);
    address feeToken = address(0);
    uint feeAmount = 0;
    UTB utb;
    UniSwapper swapper;
    VeryCoolCat cat;
    UTBExecutor utbExecutor;
    

	function setUp() public {
		// set up arbitrum chain
		string memory chain = "arbitrum";
		configureChain(
			"arbitrum", true, 42161, 0x82aF49447D8a07e3bd95BD0d56f35241523fBab1, 
			0x82aF49447D8a07e3bd95BD0d56f35241523fBab1);

		uniRouterLookup["arbitrum"] = 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45;

		tokenLookup["arbitrum"] = 0xFF970A61A04b1cA14834A43f5dE4533eBDDB5CC8;
		uniQuoterLookup["arbitrum"] = IQuoterV2(0x61fFE014bA17989E743c5F6cB21bF9697530B21e);
		utb = deployUTB(chain);
		utb.setWrapped(payable(0x82aF49447D8a07e3bd95BD0d56f35241523fBab1));
		swapper = deployUniSwapper(chain, payable(utb));
		cat = deployTheCat(chain);
		utbExecutor = deployExecutor(chain, payable(utb));
		UTBFeeCollector utbFeeCollector = deployFeeCollector(chain, payable(utb));

	}

	function testFeeCollectorRefundNotRevert() public {
		string memory chain = "arbitrum";
		switchTo(chain);

		uint256 slippage = 1;
		address  weth = wethLookup[chain];
		address  usdc = tokenLookup[chain];

		uint256 amountIn = 1e18;

		SwapParams memory swapParams = SwapParams({
			tokenIn: weth,
			amountIn: (amountIn * (100 + slippage)) / 100,
			tokenOut: address(0),
			amountOut: amountIn / 2,
			direction: 1,
			path: ""
		});

		{
			DummyContract dummy = new DummyContract();
			address payable refund = payable(address(dummy));
			SwapInstructions memory swapInstructions = SwapInstructions({
				swapperId: swapper.getId(),
				swapPayload: abi.encode(swapParams, address(utb), refund)
			});
			vm.deal(alice, swapParams.amountIn);
			vm.startPrank(alice);

			WETH(payable(weth)).deposit{value: swapParams.amountIn}();
			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))
				});
			FeeStructure memory fees = FeeStructure({
				bridgeFee: 0,
				feeAmount: feeAmount,
				feeToken: feeToken
			});
			bytes memory signature = getSignature(abi.encode(instructions, fees));
			utb.swapAndExecute(instructions, fees, signature);
			
			console.log("Balance: %s", address(utbExecutor).balance);
			assertEq(address(utbExecutor).balance, 5e17);

			vm.stopPrank();
		}
	}	

	function getSignature(
		bytes memory inputBytes
	) public pure returns (bytes memory signature) {
		bytes32 constructedHash = keccak256(
			abi.encodePacked("\x19Ethereum Signed Message:\n32", keccak256(inputBytes))
		);
		(uint8 v, bytes32 r, bytes32 s) = vm.sign(
			uint256(0xC0FFEE),
			constructedHash
		);
		signature = abi.encodePacked(r, s, v);
	}

	function deployExecutor(
		string memory chain,
		address payable utb
	) public returns (UTBExecutor _utbExecutor)
	{
		switchTo(chain);
		_utbExecutor = UTBExecutor(payable(deployContractByCode("UTBExecutor.sol:UTBExecutor")));
		_utbExecutor.transferOwnership(utb);
		UTB(utb).setExecutor(address(_utbExecutor));

	}

	function deployFeeCollector(
		string memory chain,
		address payable utb
	) public returns (UTBFeeCollector feeCollector) {
		switchTo(chain);
		feeCollector = UTBFeeCollector(payable(deployContractByCode("UTBFeeCollector.sol:UTBFeeCollector")));
		feeCollector.setUtb(utb);
		feeCollector.setSigner(vm.addr(uint256(0xC0FFEE)));
		UTB(utb).setFeeCollector(payable(address(feeCollector)));

	}


	function deployTheCat(
		string memory chain
	) public returns (VeryCoolCat cat) {
		switchTo(chain);
		cat = VeryCoolCat(payable(deployContractByCode("VeryCoolCat.sol:VeryCoolCat")));
		cat.setUsdc(tokenLookup[chain]);
		cat.setWeth(wethLookup[chain]);
	}

	function deployUTB(string memory chain) public returns (UTB utb) {
		switchTo(chain);
		utb = UTB(payable(deployContractByCode("UTB.sol:UTB")));
	}

	function deployUniSwapper(
		string memory chain,
		address payable utb
	) public returns (UniSwapper swapper) {
		switchTo(chain);
		swapper = UniSwapper(payable(deployContractByCode("UniSwapper.sol:UniSwapper")));
		swapperLookup[chain] = swapper;
		swapper.setWrapped(payable(wrappedLookup[chain]));
		swapper.setRouter(payable(uniRouterLookup[chain]));
		swapper.setUtb(utb);
		UTB(utb).registerSwapper(address(swapper));
	}

	function deployContractByCode(string memory code) internal returns (address deployed) {
		bytes memory bytecode = vm.getCode(code);
		bytes memory data = bytes.concat(bytecode, "");
		assembly {
			deployed := create(0, add(data, 0x20), mload(data))
		}
		if (deployed == address(0)) {
			revert("Fail to deploy");
		}
	}


	function switchTo(string memory chain) internal {
		// Configure chain
		uint256 forkId = forkLookup[chain];
		vm.selectFork(forkId);

		if (block.chainid != chainIdLookup[chain]) {
			revert(string.concat("chainId mismatch for chain: ", chain));
		}

	}

	function configureChain(
		string memory chain,
		bool isGasEth,
		uint256 chainId,
		address weth,
		address wrapped
	) public {
		try vm.createFork(chain) returns (uint256 forkId) {
			forkLookup[chain] = forkId;
		} catch {}

		gasEthLookup[chain] = isGasEth;
		vm.label(weth, string.concat(chain, "_WETH"));
		wethLookup[chain] = weth;
		if (weth != wrapped) {
			vm.label(wrapped, string.concat(chain, "_WRAPPED"));
		}
		wrappedLookup[chain] = wrapped;
		chainIdLookup[chain] = chainId;

	}
}

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend either reverting if refund.call fails or transferring wrapped native tokens to refund instead.

Assessed type

ETH-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #25

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid