santiellena - `GoatV1Pair` may never enter pre-sale period due to DoS

[sherlock-admin4]

santiellena

medium

GoatV1Pair may never enter pre-sale period due to DoS

Summary

An attacker could send 1 wei of tokens to the pair before first minting, preventing the pair to enter in pre-sale mode because of a strict balance of tokens condition.

Vulnerability Detail

On the first call to GoatV1Pair::mint when a team wants to start the pre-sale mode, meaning that _vestingUntil == _MAX_UINT32 and balanceEth < mintVars.bootstrapEth, a strict check on the balance of tokens in the pair is done: https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1Pair.sol#L139-L141

        if (balanceToken != (tokenAmtForPresale + tokenAmtForAmm)) {
            revert GoatErrors.InsufficientTokenAmount();
        }

The transaction will revert even if the difference is of 1 wei which creates a cheap scenario for DoS. Additionally, as (tokenAmtForPresale + tokenAmtForAmm) are values calculated from internal pair variables, the solution to this won't be just entering new parameters (even if that were the case, modifying "price" parameters modifying teams plans won't be expected behavior).

Impact

GoatV1Pair's may never enter in pre-sale period.

Code Snippet

https://github.com/sherlock-audit/2024-03-goat-trading/blob/beb09519ad0c0ec0fdf5b96060fe5e4aafd71cff/goat-trading/contracts/exchange/GoatV1Pair.sol#L139-L141

Tool used

Manual Review

Recommendation

Implement a less strict check on token balance:

-       if (balanceToken != (tokenAmtForPresale + tokenAmtForAmm)) {
+       if (balanceToken < (tokenAmtForPresale + tokenAmtForAmm)) {
            revert GoatErrors.InsufficientTokenAmount();
        }

If the system needs that strict equality, consider sending surplus tokens to a trusted address or burning them.

[sherlock-admin3]

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

tsvetanovv commented:

This is known issue. Check Readme.

[santiellena]

Escalate

This is not a known issue provided on the Readme file:

1) Contracts are not set-up for Blast network or zkSync.
2) 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.
3) Blocklist token problems are understood and not a concern.
4) Tx failures and griefing resulting from MEV protection are known and accepted.
5) DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

If the duplicated considered by the judge is number 5, it is not correct. This is not an issue related to the user selling their tokens during the vesting period (after the presale period), this is about right before the pool enters the presale period (before there is a initial liquidity provider). This will cause a DoS of the pool and not a griefing to the user selling tokens. The explanation of the known issue can be found on the public audit provided by the protocol team. Check [M-4] issue, that's the known issue, and the root cause is completely different.

[sherlock-admin2]

Escalate

This is not a known issue provided on the Readme file:

1) Contracts are not set-up for Blast network or zkSync.
2) 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.
3) Blocklist token problems are understood and not a concern.
4) Tx failures and griefing resulting from MEV protection are known and accepted.
5) DOS during bootstrapping by sending 1 wei of token directly to the pair right before a user attempts to sell their whole token balance.

If the duplicated considered by the judge is number 5, it is not correct. This is not an issue related to the user selling their tokens during the vesting period (after the presale period), this is about right before the pool enters the presale period (before there is a initial liquidity provider). This will cause a DoS of the pool and not a griefing to the user selling tokens. The explanation of the known issue can be found on the public audit provided by the protocol team. Check [M-4] issue, that's the known issue, and the root cause is completely different.

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]

By known issue, I meant 2 and 5. I think this comment of mine applies here as well.

[santiellena]

Thanks for the answer!

I'm pretty sure that known issue number 2 does not refer to the one provided here. Here is an explanation on what known issue number 2 means: (image taken from Discord public chat)

I would suggest to reach the sponsor and ask them. I just don't want an issue not being fixed because of a misinterpretation. @chiranz

[Evert0x]

@cvetanovv do you have a response here?

[chiranz]

To be fair this issue surfaced in one of our internal audit. I think we missed to phrase it well in the known bugs section. I think we should keep this as valid and pay the auditor.

[Evert0x]

Planning to accept escalation and make Medium severity. I believe the known issue section doesn't apply here.

Based on the impact section of the report this is a serious issue. Happy to hear counter arguments (cc @cvetanovv )

[cvetanovv]

I agree it is a valid bug. I was just misled that it was a "known issue" and therefore left it invalid. We can accept the escalation.

[Evert0x]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• santiellena: rejected

[zzykxx]

@Evert0x the impact stated here is incorrect, there is no permanent DOS. What would happen if an attacker donated 1 wei by frontrunning mint() is that the transaction would rever. The team could simply call mint() again and correctly mint liquidity and have the pool enter in pre-sale period. Keep in mind that mint() has to be called from a function that correctly implements safety checks as stated in the comments.

According to sherlock rules this is not a valid medium severity issue:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

[cvetanovv]

@Evert0x the impact stated here is incorrect, there is no permanent DOS. What would happen if an attacker donated 1 wei by frontrunning mint() is that the transaction would rever. The team could simply call mint() again and correctly mint liquidity and have the pool enter in pre-sale period. Keep in mind that mint() has to be called from a function that correctly implements safety checks as stated in the comments.

According to sherlock rules this is not a valid medium severity issue:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

At first, I was thinking exactly what you wrote so I left it invalid. You can see my comment above that leads to this comment. If we don't have a permanent DOS(more than a week) then we need to change it to Invalid.

[Evert0x]

Thanks for providing the extra context.

After taking another look I believe the right decision is to invalidate this issue. The following judging rule applies

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

This is a true statement

The team could simply call mint() again and correctly mint liquidity and have the pool enter in pre-sale period.

Planning to close the issue and change the escalation status to rejected.

[adamidarrha]

@Evert0x, what @zzykxx mentioned is correct. The issue doesn't cause a DOS for more than one transaction; it only results in the temporary reverting of adding liquidity transactions. These transactions can always be resubmitted normally, or one could use Flashbots to circumvent the problem.

[santiellena]

You are right and I explained myself poorly. I'm sorry for that. The permanent DoS will occur only if the balance of tokens is greater than the balance needed from the initial liquidity provider. So you are right, it will not take 1 wei, it will take (tokenAmtForPresale + tokenAmtForAmm).