sherlock-audit / 2024-03-goat-trading-judging

3 stars 2 forks source link

zzykxx - The `takeOverPool()` can be frontrun to steal funds #65

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 8 months ago

zzykxx

high

The takeOverPool() can be frontrun to steal funds

Summary

The takeOverPool() requires to transfer tokens and WETH in the pool before being called and it can be front-run to steal funds.

Vulnerability Detail

The takeOverPool() function allows a team to take over a pool from malicious actors. To do so the caller is required to transfer tokens and WETH to the pair before calling takeOverPool().

Because there is a delay between when the tokens and WETH are transferred and the call to takeOverPool() and attacker can insert a transaction in-between to steal funds. This can be done with a call to swap().

Note that the router does not implement any function that allows to call takeOverPool() safely.

Impact

A malicious actor can steal funds and prevent a pool from being taken over.

Code Snippet

Tool used

Manual Review

Recommendation

Implement a function in the router that performs correct safety checks and allows a team to safely call takeOverPool().

chiranz commented 7 months ago

This function is meant to be called by sophisticated users who know that there should be safety measures taken before calling that function.. It was our choice not to have takeOverPool() in the router.

zzykxx commented 7 months ago

Escalate

Not having a way to call takeOverPool() with the correct safety checks puts funds at risk. As far as I know it was not mentioned anywhere that users should "know that there should be safety measures taken before calling that function".

sherlock-admin2 commented 7 months ago

Escalate

Not having a way to call takeOverPool() with the correct safety checks puts funds at risk. As far as I know it was not mentioned anywhere that users should "know that there should be safety measures taken before calling that function".

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.

chiranz commented 7 months ago

As we have explicitly stated that the functions should be called with sufficient safety checks. Even though the auditor is valid in his claims this should not be a high but medium.

cvetanovv commented 7 months ago

@chiranz Can you point me to where there is a warning to the user about this risk?

Because it looks like a valid High to me. But if the user knows about this risk then we can count it as a known issue.

chiranz commented 7 months ago

Warning is here.... https://github.com/sherlock-audit/2024-03-goat-trading/blob/main/goat-trading/contracts/exchange/GoatV1Pair.sol#L21

cvetanovv commented 7 months ago

According to Sherlock's rules, we can count it as a known issue: "Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments)"

So we can reject the escalation and this issue can remain invalid.

zzykxx commented 7 months ago

I missed that comment at the top of the contract. In all the other functions it's explicitly stated that it should be called from a contract that implements safety checks, except for takeOverPool() which is why I submitted the issue in the first place (plus, the router doesn't implement a safe way to call takeOverPool()). I agree that according to the rules this should be invalid.

chiranz commented 7 months ago

I should have made it explicit at the function level too. Gonna update comments at function level too.

Evert0x commented 7 months ago

Result: Invalid Unique

sherlock-admin3 commented 7 months ago

Escalations have been resolved successfully!

Escalation status: