0xhashiman - Loss of funds for liquidity providers

[sherlock-admin4]

0xhashiman

high

Loss of funds for liquidity providers

Summary

The issue lies within the GoatV1Pair contract, specifically in the way the project handles casting. Let's examine the code of the function_update():

    function _update(uint256 balanceEth, uint256 balanceToken, bool deductFees) internal {
        // Update token reserves and other necessary data
        if (deductFees) {
            _reserveEth = uint112(balanceEth - (_pendingLiquidityFees + _pendingProtocolFees));
        } else {
            _reserveEth = uint112(balanceEth);
        }
        _reserveToken = uint112(balanceToken);
    }

This function updates the reserves of ETH and the token based on the balances existing in the pool. However, currently, the project is casting from uint256 balanceToken to uint112(balanceToken). _reserveToken represents the balance of the pair and how much it holds of a specific token. Unsafe casting means that if the value of balanceToken is larger than uint112, this will silently overflow and cause _reserveTokento be 0. This function is also used in every function such as swap, burn, andmint. Having 0 as _reserveToken when there is a balance will result in a massive DOS.

Vulnerability Detail

The vulnerability exists when users creates pool using high total supply tokens with values more than max uint112, since on the contest details the project is accepting any issues from ERC20 tokens because they accept all kind of tokens.

Quote from contest page:

Which ERC20 tokens do you expect will interact with the smart contracts?

Any ERC20 tokens should be able to be used.

If someone creates an LP using high/big total supply tokens such as memecoins for example, the first person to add liquidity will lose all of the liquidity added he also cant remove liquidity .

Lets break down this step by step: 1) User will create any ERC20 token with high total supply 2) User will create pair and addliquidity using all the total supply of the token. 3) The balance of the pair will be more than uint112 because the _update accepts uint256 as balance of token and cast it to a lower uint. 4) When this line is executed:_reserveToken = uint112(balanceToken), the value of _reserveToken becomes 0. 5) Now, for the user who added liquidity, nothing appears to have gone wrong because this overflow reverts silently. When users attempt to swap, their transaction will revert with "InsufficientAmountOut" because the swap function calls _getActualReserves to get the initialReserveToken, which is now 0 because initialReserveToken = _reserveToken. In the swap function, we check if the amount of this operation is greater than the reserve of either ETH or the token, reverting with "InsufficientAmountOut."

   function swap(uint256 amountTokenOut, uint256 amountWethOut, address to) external nonReentrant {
        ...
        ..
        (swapVars.initialReserveEth, swapVars.initialReserveToken) = _getActualReserves();
        if (amountTokenOut > swapVars.initialReserveToken || amountWethOut > swapVars.initialReserveEth) {
            revert GoatErrors.InsufficientAmountOut();
        }
        }

6) Since the swap is now reverted and no one can trade this token, if liquidity providers want to remove liquidity, this transaction will also fail with "InsufficientLiquidityBurned." This is because the burn function calculates the amountToken based on _reserveToken, which is 0 in this case.

   function burn(address to) external returns (uint256 amountWeth, uint256 amountToken) {
        uint256 liquidity = balanceOf(address(this));
    ...
        uint256 totalSupply_ = totalSupply();
        amountWeth = (liquidity * _reserveEth) / totalSupply_;
        amountToken = (liquidity * _reserveToken) / totalSupply_;
        if (amountWeth == 0 || amountToken == 0) {
            revert GoatErrors.InsufficientLiquidityBurned();
        }
    ...
    }

POC Details

### Token Code Add this code in MockERC20.sol. ```solidity contract TokenBigSupp is ERC20 { constructor() ERC20("BigSup", "BGS") { _mint(msg.sender, 100000000000000* 10 ** 18); } function mint(address to, uint256 amount) public { _mint(to, amount); } } ``` ### SetUp We need to first deploy the token in the setUp() function in BaseTest.t.sol. ```solidity tokenBigSupp = new TokenBigSupp(); ``` Then in the same file we need to add two function so that our POC work 1)This addLiquidityParamsForPoc function is for setuping all the initial param for the liquidity to handle both cases before presale and after presale. ```solidity function addLiquidityParamsForPoc(bool initial, bool sendInitWeth) public returns (AddLiquidityParams memory) { weth.deposit{value: 100e18}(); if (initial) { /* ------------------------------- SET PARAMS ------------------------------- */ addLiqParams.token = address(tokenBigSupp); addLiqParams.tokenDesired = 0; addLiqParams.wethDesired = 0; addLiqParams.tokenMin = 0; addLiqParams.wethMin = 0; addLiqParams.to = address(this); addLiqParams.deadline = block.timestamp + 1000; addLiqParams.initParams = GoatTypes.InitParams(10e18, 10e18, sendInitWeth ? 5e18 : 0, 100000000000000e18); } else { addLiqParams.token = address(tokenBigSupp); addLiqParams.tokenDesired = 1000000000e18; addLiqParams.wethDesired = 1e18; addLiqParams.tokenMin = 0; addLiqParams.wethMin = 0; addLiqParams.to = address(this); addLiqParams.deadline = block.timestamp + 1000; addLiqParams.initParams = GoatTypes.InitParams(0, 0, 0, 0); } return addLiqParams; } ``` 2)This second function will be called the first time to add liquidity into the pool. ```solidity function _addLiquidityAndConvertToAmmForPoc() internal returns (uint256 tokenAmtUsed, uint256 wethAmtUsed, uint256 liquidity, uint256 actualTokenAmountToSend) { addLiquidityParamsForPoc(true, true); addLiqParams.initParams.initialEth = 10e18; // set all weth actualTokenAmountToSend = router.getActualBootstrapTokenAmount( addLiqParams.initParams.virtualEth, addLiqParams.initParams.bootstrapEth, addLiqParams.initParams.initialEth, addLiqParams.initParams.initialTokenMatch ); tokenBigSupp.approve(address(router), 1000000000000000000000e18); weth.approve(address(router), addLiqParams.initParams.initialEth); (tokenAmtUsed, wethAmtUsed, liquidity) = router.addLiquidity( addLiqParams.token, addLiqParams.tokenDesired, addLiqParams.wethDesired, addLiqParams.tokenMin, addLiqParams.wethMin, addLiqParams.to, addLiqParams.deadline, addLiqParams.initParams ); } ``` ### Code Add this function in GoatV1Router.t.sol so that we can emitate how a normal user will add liquidity and not directly interact with the pair. The function below will revert with InsufficientLiquidityBurned. ```solidity function testRemoveLiquidityUsingHighTotalSupTokens() public { _addLiquidityAndConvertToAmmForPoc(); GoatV1Pair pair = GoatV1Pair(factory.getPool(addLiqParams.token)); uint256 lpTotalSupply = pair.totalSupply(); //AT THIS POINT PRESALE IS ENDED addLiqParams = addLiquidityParamsForPoc(false, false); // new params // mint tokens to lp tokenBigSupp.mint(lp_1, 1000000000e18); weth.transfer(lp_1, 1e18); // Lp provides liqudity vm.startPrank(lp_1); tokenBigSupp.approve(address(router), 1000000000e18); weth.approve(address(router), 1e18); addLiqParams.to = lp_1; // change to lp // (uint256 reserveEth, uint256 reserveToken) = pair.getReserves(); // get reserves before adding liquidity to check for Lp minted later (uint256 tokenAmtUsed, uint256 wethAmtUsed, uint256 liquidity) = router.addLiquidity( addLiqParams.token, addLiqParams.tokenDesired, addLiqParams.wethDesired, addLiqParams.tokenMin, addLiqParams.wethMin, addLiqParams.to, addLiqParams.deadline, addLiqParams.initParams ); vm.warp(block.timestamp + 2 days); pair.approve(address(router),100); router.removeLiquidity(address(tokenBigSupp), 100, 0, 0, lp_1, block.timestamp); vm.stopPrank(); } ```

Impact

This unsafe casting will result in a massive DOS for all liquidity providers , they wont be able to remove liquidity and get their original funds back once this issue happens.

Code Snippet

https://github.com/sherlock-audit/2024-03-goat-trading/blob/main/goat-trading/contracts/exchange/GoatV1Pair.sol#L633-L641

Tool used

Manual Review

Recommendation

Consider verifying that the values are within the acceptable range before casting, or use openzepplin safeCasting library.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

this should be invalid

[0xhashiman]

Escalate I think there might have been a mistake in how this issue was judged. It was labeled as a duplicate of #22 , but upon verification, this isn't correct. My issue revolves around the unsafe casting in _update() from uint256 to uint112, not the loss of Treasury Fees in_handleFees(). I've confirmed this issue using the POC provided in the report, since liquidity providers are unable to remove liquidity due to this unsafe conversion. I've marked this issue as high, since it represents a risk of funds loss for liquidity providers, when dealing with tokens of significant total supply.

[sherlock-admin2]

Escalate I think there might have been a mistake in how this issue was judged. It was labeled as a duplicate of #22 , but upon verification, this isn't correct. My issue revolves around the unsafe casting in _update() from uint256 to uint112, not the loss of Treasury Fees in_handleFees(). I've confirmed this issue using the POC provided in the report, since liquidity providers are unable to remove liquidity due to this unsafe conversion. I've marked this issue as high, since it represents a risk of funds loss for liquidity providers, when dealing with tokens of significant total supply.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[cvetanovv]

Escalate I think there might have been a mistake in how this issue was judged. It was labeled as a duplicate of #22 , but upon verification, this isn't correct. My issue revolves around the unsafe casting in _update() from uint256 to uint112, not the loss of Treasury Fees in_handleFees(). I've confirmed this issue using the POC provided in the report, since liquidity providers are unable to remove liquidity due to this unsafe conversion. I've marked this issue as high, since it represents a risk of funds loss for liquidity providers, when dealing with tokens of significant total supply.

Escalate

Escalate on behalf of @0xhashiman

[sherlock-admin2]

Escalate I think there might have been a mistake in how this issue was judged. It was labeled as a duplicate of #22 , but upon verification, this isn't correct. My issue revolves around the unsafe casting in _update() from uint256 to uint112, not the loss of Treasury Fees in_handleFees(). I've confirmed this issue using the POC provided in the report, since liquidity providers are unable to remove liquidity due to this unsafe conversion. I've marked this issue as high, since it represents a risk of funds loss for liquidity providers, when dealing with tokens of significant total supply.

Escalate

Escalate on behalf of @0xhashiman

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 have added the two issues as duplicates because of the "unsafe cast" root, but I agree that they differ and can be separated.

However, the chance of this unsafe casting happening is almost impossible. It would only happen with some non-standard meme coin as you say. Even then the chance of exceeding that value is minimal.

The protocol field also did not specify to use such tokens - https://github.com/sherlock-audit/2024-03-goat-trading?tab=readme-ov-file#q-do-you-expect-to-use-any-of-the-following-tokens-with-non-standard-behaviour-with-the-smart-contracts.

Unlikely as they wrote: "but would appreciate feedback if there are important ones that could cause big problems" they mean such a non-standard token with totalSupply hundreds of millions.

Therefore, I think the escalation should be rejected, and this issue should remain information/invalid.

[0xhashiman]

I have added the two issues as duplicates because of the "unsafe cast" root, but I agree that they differ and can be separated.

However, the chance of this unsafe casting happening is almost impossible. It would only happen with some non-standard meme coin as you say. Even then the chance of exceeding that value is minimal.

The protocol field also did not specify to use such tokens - https://github.com/sherlock-audit/2024-03-goat-trading?tab=readme-ov-file#q-do-you-expect-to-use-any-of-the-following-tokens-with-non-standard-behaviour-with-the-smart-contracts.

Unlikely as they wrote: "but would appreciate feedback if there are important ones that could cause big problems" they mean such a non-standard token with totalSupply hundreds of millions.

Therefore, I think the escalation should be rejected, and this issue should remain information/invalid.

I don't entirely agree with your point, the likelihood of investors launching liquidity pools with high token supply is always present since no warning is made from the project side for people to avoid using these kind of tokens in the protocol, the project cannot control people's tokens; they need to operate on a universal level. Their is not a standard for memcoins they are simple ERC20 i just illustrated my issue with that particular example. However, the function I've flagged in this issue which is vulnerable to this attack _update() is similar on how the uniswap v2 pair _update() work and you can clearly see that they do indeed check for this overflow at line 74.

[Evert0x]

I believe this issue applies to tokens with non-standard behavior and shouldn't be rewarded.

Planning to remove duplication status and invalidate issue

[0xhashiman]

I believe this issue applies to tokens with non-standard behavior and shouldn't be rewarded.

Planning to remove duplication status and invalidate issue

Could you clarify what you mean by "non-standard tokens"? This issue revolves around an ERC20 token that has all the functionalities of a typical token but with a high total supply. I'm uncertain why you labeled this as non-standard behavior since i didnt use any ERC777 or tokens that returns false on failure instead of revert.

[Evert0x]

To exceed uint112 as a total supply (assuming 18 decimals) you need five quadrillion one hundred ninety-two trillion two hundred ninety-six billion eight hundred fifty-eight million five hundred thirty-four thousand eight hundred twenty-eight tokens.

My judgment is that tokens with this amount of total supply are not standard

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cvetanovv: rejected