FassiSecurity - Malicious user can cause mint to fail

[sherlock-admin4]

FassiSecurity

high

Malicious user can cause mint to fail

Summary

the mint function allows users to add liquidity to a pool, and in return, they receive liquidity tokens. It will go through several checks, 1 of which is a check to ensure that the balancetoken is equal to tokenAmtForPresale + tokenAmtForAmm.

Vulnerability Detail

  function mint(address to) external nonReentrant returns (uint256 liquidity) {
        uint256 totalSupply_ = totalSupply();
        uint256 amountWeth;
        uint256 amountToken;
        uint256 balanceEth = IERC20(_weth).balanceOf(address(this));
        uint256 balanceToken = IERC20(_token).balanceOf(address(this));

        GoatTypes.LocalVariables_MintLiquidity memory mintVars;

        mintVars.virtualEth = _virtualEth;
        mintVars.initialTokenMatch = _initialTokenMatch;
        mintVars.bootstrapEth = _bootstrapEth;

        if (_vestingUntil == _MAX_UINT32) {
            // Do not allow to add liquidity in presale period
            if (totalSupply_ > 0) revert GoatErrors.PresalePeriod();
            // don't allow to send more eth than bootstrap eth
            if (balanceEth > mintVars.bootstrapEth) {
                revert GoatErrors.SupplyMoreThanBootstrapEth();
            }

            if (balanceEth < mintVars.bootstrapEth) {
                (uint256 tokenAmtForPresale, uint256 tokenAmtForAmm) = _tokenAmountsForLiquidityBootstrap(
                    mintVars.virtualEth, mintVars.bootstrapEth, balanceEth, mintVars.initialTokenMatch
                );
          ->     if (balanceToken != (tokenAmtForPresale + tokenAmtForAmm)) {
                    revert GoatErrors.InsufficientTokenAmount();
    //..Ommitted code

tokenAmtForPresale & tokenAmtForAmm are both retrieved after calling _tokenAmountsForLiquidityBootstrap

   function _tokenAmountsForLiquidityBootstrap(
        uint256 virtualEth,
        uint256 bootstrapEth,
        uint256 initialEth,
        uint256 initialTokenMatch
    ) internal pure returns (uint256 tokenAmtForPresale, uint256 tokenAmtForAmm) {
        uint256 k = virtualEth * initialTokenMatch;
        tokenAmtForPresale = initialTokenMatch - (k / (virtualEth + bootstrapEth));
        uint256 totalEth = virtualEth + bootstrapEth;
        tokenAmtForAmm = (k * bootstrapEth) / (totalEth * totalEth);

        if (initialEth != 0) {
            uint256 numerator = (initialEth * initialTokenMatch);
            uint256 denominator = virtualEth + initialEth;
            uint256 tokenAmountOut = numerator / denominator;
            tokenAmtForPresale -= tokenAmountOut;
        }
    }

As we can see, the _tokenAmountsForLiquidityBootstrap takes multiple parameters `(virtualEth, boostrapEth, initialEth, and initialTokenMatch).

Impact

We will now demonstrate how a malicious user can always cause the if statement to trigger, leading to an honest user being unable to provide liquidity:

• mint function is called
• malicious user sees this and sends 1 wei worth of token towards balanceToken
• mint function will execute until it reaches the aforementioned if statement
• This will now fail since, due to the donation attack, balanceToken will not be the same as tokenAmtForPresale + tokenAmtForAmm.
• Malicious user can keep repeating this and cause DOS.

We are aware of the known issue stated in the docs:

DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

However, they are not the same. The known issue describes a donation attack occurring when a user attempts to sell their token balance, whereas this issue describes a DOS when a user tries to provide liquidity.

Code Snippet

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

Tool used

Manual Review

Recommendation

Implement some sort of logic which prevents such an attack

[sherlock-admin3]

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

tsvetanovv commented:

This is known issue. Check Readme.

[bronzepickaxe]

Escalate

These are the known issues in the README.md:

Please list any known issues/acceptable risks that should not result in a valid finding.

    Contracts are not set-up for Blast network or zkSync.
    Temporary LP DOS by minting/burning tokens to an LP. We've added protection to make this less effective if it's a small amount, but otherwise accept that it can be done. If it can be made permanent, cost effective, and can't be defended against with a private RPC then that would be a legitimate bug.
    Blocklist token problems are understood and not a concern.
    Tx failures and griefing resulting from MEV protection are known and accepted.
    DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

We are not sure which one you are alluding to, if it is:

    DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

then this is not applicable to our report. Our report does not mention a user selling their token balance nor does it mention sending 1 wei of token directly to the pair.

Therefore, this should be treated as in-scope.

Thanks!

[sherlock-admin2]

Escalate

These are the known issues in the README.md:

Please list any known issues/acceptable risks that should not result in a valid finding.

    Contracts are not set-up for Blast network or zkSync.
    Temporary LP DOS by minting/burning tokens to an LP. We've added protection to make this less effective if it's a small amount, but otherwise accept that it can be done. If it can be made permanent, cost effective, and can't be defended against with a private RPC then that would be a legitimate bug.
    Blocklist token problems are understood and not a concern.
    Tx failures and griefing resulting from MEV protection are known and accepted.
    DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

We are not sure which one you are alluding to, if it is:

    DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

then this is not applicable to our report. Our report does not mention a user selling their token balance nor does it mention sending 1 wei of token directly to the pair.

Therefore, this should be treated as in-scope.

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 disagree with the escalation. You can see 2. from Known issue: Temporary LP DOS by minting/burning tokens to an LP.....

Separately, such short-term DOS attacks are Low according to the Sherlock documentation. They would only be valid if used on some time-sensitive function like Auction for example.

[santiellena]

Known issue 2 refers to the ability to lock temporarily all the tokens of a LP by minting the LP some tokens. Here is the code that does that.

I submitted and escalated an issue with this exact root cause mentioned by @bronzepickaxe . However, the impact I described in #79 is different (permanent DoS).

[Evert0x]

Planning to reject escalation and keep issue state as is, known issue.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• bronzepickaxe: rejected