sherlock-audit / 2023-12-footium-judging

7 stars 6 forks source link

detectiveking - FootiumClub NFT Seller can Honeypot Buyers #53

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 10 months ago

detectiveking

high

FootiumClub NFT Seller can Honeypot Buyers

Summary

This issue is similar to https://github.com/sherlock-audit/2023-04-footium-judging/issues/291

A FootiumClub NFT seller can honeypot buyers by making them think that there are good academy player NFTs for them to mint after they acquire the club NFT, but then frontrun and mint the academy player NFTs before the buy.

Vulnerability Detail

The attack path is as follows:

Requirement: You need a club NFT that has reasonably good mintable academy players

  1. List the club NFT for sale
  2. Someone sees it and pays a high price for it, after seeing they would be able to mint good academy players after acquiring the club
  3. Seller sees the buy transaction in the mempool, and frontruns it with a mint to all the academy player NFTs associated with the club. The academy player NFTs are sent to the malicious seller's address.
  4. Buyer is left with the club NFT but cannot mint any of the valuable academy NFTs, so they have lost a good amount on their investment.

Impact

Malicious club NFT seller can honeypot buyers

Code Snippet

https://github.com/sherlock-audit/2023-12-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L96-L138

Tool used

Manual Review

Recommendation

Implement ERC 721 timelock

Duplicate of #48

sherlock-admin2 commented 9 months ago

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

darkart commented:

Players are not tied directly to the club according to developers

EV_om commented:

Duplicate of #014, see comments

detectiveking123 commented 9 months ago

Escalate

This issue, as well its two duplicates, are valid

To respond to the judge's concerns described here (https://github.com/sherlock-audit/2023-12-footium-judging/issues/48)

  1. I believe there is a slight misunderstanding here. You are correct in that if you own a club, you do not own the players. However, you do own the right to mint certain academy players, which is what this issue is referring to. Here is a public statement from the sponsor directly addressing this (near the end of the text):
Screen Shot 2023-12-21 at 8 46 19 PM

You can also see this if you read mintPlayer in FootiumAcademy.sol carefully. The code is as follows:

    function mintPlayer(
        uint256 clubId,
        string calldata playerId,
        bytes32[] calldata mintProof
    ) external payable whenNotPaused nonReentrant {

You just need a clubId, playerId, and proof to mint.

So there is no point in front-running to mint players, since the original club owner before sale still owns all the players within the club and the buyer can use this players to compete in the game. <- This statement is actually incorrect; the mintable academy players would actually belong to the club, as you can see from the above code.

  1. You are correct that mempool front-running is not an issue on Arbitrum. However, this issue (https://github.com/sherlock-audit/2023-04-footium-judging/issues/291) was judged as high in spite of this. You can still frontrun if you can guess roughly when the victim will send the transaction.
  2. Another judge brought up a concern that the mintable academy players for a club may not be shown in the UI. I believe it should be, as it is part of the clubs overall value (as confirmed by the sponsors) and sellers should know it before they buy. Even if it is not, the malicious seller can publish the merkle proofs in the NFT description for example.

Overall, I do believe this issue is valid, but I believe it to be a medium rather than a high. Ultimately up to @Czar102 though.

sherlock-admin2 commented 9 months ago

Escalate

This issue, as well its two duplicates, are valid

To respond to the judge's concerns described here (https://github.com/sherlock-audit/2023-12-footium-judging/issues/48)

  1. I believe there is a slight misunderstanding here. You are correct in that if you own a club, you do not own the players. However, you do own the right to mint certain academy players, which is what this issue is referring to. Here is a public statement from the sponsor directly addressing this (near the end of the text):
Screen Shot 2023-12-21 at 8 46 19 PM

You can also see this if you read mintPlayer in FootiumAcademy.sol carefully. The code is as follows:

    function mintPlayer(
        uint256 clubId,
        string calldata playerId,
        bytes32[] calldata mintProof
    ) external payable whenNotPaused nonReentrant {

You just need a clubId, playerId, and proof to mint.

So there is no point in front-running to mint players, since the original club owner before sale still owns all the players within the club and the buyer can use this players to compete in the game. <- This statement is actually incorrect; the mintable academy players would actually belong to the club, as you can see from the above code.

  1. You are correct that mempool front-running is not an issue on Arbitrum. However, this issue (https://github.com/sherlock-audit/2023-04-footium-judging/issues/291) was judged as high in spite of this. You can still frontrun if you can guess roughly when the victim will send the transaction.
  2. Another judge brought up a concern that the mintable academy players for a club may not be shown in the UI. I believe it should be, as it is part of the clubs overall value (as confirmed by the sponsors) and sellers should know it before they buy. Even if it is not, the malicious seller can publish the merkle proofs in the NFT description for example.

Overall, I do believe this issue is valid, but I believe it to be a medium rather than a high. Ultimately up to @Czar102 though.

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.

nevillehuang commented 9 months ago

Escalate

This issue, as well its two duplicates, are valid

To respond to the judge's concerns described here (#48)

  1. I believe there is a slight misunderstanding here. You are correct in that if you own a club, you do not own the players. However, you do own the right to mint certain academy players, which is what this issue is referring to. Here is a public statement from the sponsor directly addressing this (near the end of the text):
Screen Shot 2023-12-21 at 8 46 19 PM

You can also see this if you read mintPlayer in FootiumAcademy.sol carefully. The code is as follows:

    function mintPlayer(
        uint256 clubId,
        string calldata playerId,
        bytes32[] calldata mintProof
    ) external payable whenNotPaused nonReentrant {

You just need a clubId, playerId, and proof to mint.

So there is no point in front-running to mint players, since the original club owner before sale still owns all the players within the club and the buyer can use this players to compete in the game. <- This statement is actually incorrect; the mintable academy players would actually belong to the club, as you can see from the above code.

  1. You are correct that mempool front-running is not an issue on Arbitrum. However, this issue (0x52 - Malicious users can honeypot other users by transferring out ERC20 and ERC721 tokens right before sale 2023-04-footium-judging#291) was judged as high in spite of this. You can still frontrun if you can guess roughly when the victim will send the transaction.
  2. Another judge brought up a concern that the mintable academy players for a club may not be shown in the UI. I believe it should be, as it is part of the clubs overall value (as confirmed by the sponsors) and sellers should know it before they buy. Even if it is not, the malicious seller can publish the merkle proofs in the NFT description for example.

Overall, I do believe this issue is valid, but I believe it to be a medium rather than a high. Ultimately up to @Czar102 though.

Because front-running is a non-issue on arbitrum all other reason shouldn’t matter unless sponsor wants to clarify. “Guessing transactions” is not a valid reason for validating.

detectiveking123 commented 9 months ago

By that logic https://github.com/sherlock-audit/2023-04-footium-judging/issues/291 should be invalid

I said that mempool front-running isn't an issue. There's a difference between mem-pool front-running and general front-running. Mempool front-running is seeing a transaction in the mempool and front-running it. General front-running can be done in many ways -- what I described to you is a valid way to front-run and you see it with token snipers all the time.

detectiveking123 commented 9 months ago

Here's a valid attack for example:

  1. You have a club that has the right to mint 10 highly coveted legendary academy players. The club + 10 academy players is easily worth 10 ETH total.
  2. You put the club NFT up for sale at 8 ETH, knowing it will surely be almost immediately taken.
  3. You guess it will be taken in 30 seconds and are correct and land on the same block -- you mint the legendary academy players for yourself but the buy still goes through right afterward, so the buyer is down a lot of money.
nevillehuang commented 9 months ago

@detectiveking123 fair point,

@Czar102 why was the previous issue here accepted as an issue when there is no mempool on arbitrum? To me both issues should be invalid. To my knowledge, front-running issues in Arbitrum has always been rejected in both sherlock and other platforms such as C4 historically. See here:

The Arbitrum Sequencer orders transactions on a first come, first served basis; the Sequencer inserts transactions into a queue based on the order they are received and executes them accordingly. This queue thus exists in lieu of a mempool. The Sequencer's queue has no space limit; transactions on the queue will eventually timeout and be discarded if not executed in time. Under normal conditions, the queue is empty, since transactions are executed near-instantaneously.

There is no mempool so how do you even make a reasonable guess to front-run? In the context of this protocol, it is certainly not profitable.

detectiveking123 commented 9 months ago

There is no mempool so how do you even make a reasonable guess to front-run? In the context of this protocol, it is certainly not profitable.

As described above, if I were the attacker, I'd give a good price so someone would be baited into buying almost instantly. I can then narrow down the time that they'll buy to sometime in the next few minutes, which would give me a nontrivial chance of frontrunning them.

nevillehuang commented 9 months ago

There is no mempool so how do you even make a reasonable guess to front-run? In the context of this protocol, it is certainly not profitable.

As described above, if I were the attacker, I'd give a good price so someone would be baited into buying almost instantly. I can then narrow down the time that they'll buy to sometime in the next few minutes, which would give me a nontrivial chance of frontrunning them.

To me this is just speculating on when the buyer will actually submit the transaction to buy. Imo, the guessing game is too difficult to warrant this as a medium severity finding, even more so when you don't know which transactions will overtake you since there are so many transactions submitted to the arbitrum sequencer.

detectiveking123 commented 9 months ago

My opinion is that issues where the malicious attacker needs to see the user transaction before constructing their own transaction should be invalid on Arbitrum, since mempool frontrunning is nonexistent (though frontrunning does exist!).

However, in this case, the attacker can send their own front-run transaction without even seeing the user transaction (but they just have to get lucky with placement). This distinction turns this issue into a valid attack, in my opinion.

nevillehuang commented 9 months ago

@detectiveking123

The seller of the club needs to see the buyer sending the transaction to buy the NFT so I think your points contradict each other.

detectiveking123 commented 9 months ago

That's not what I mean, malicious sender can still construct their transaction without seeing the user transaction.

nevillehuang commented 9 months ago

@detectiveking123 Im still unsure what you are trying to explain here but I believe my points in our discussion are sufficient to support my stance of invalidation. Will leave it to @Czar102 to decide final validity.

Czar102 commented 9 months ago

Are the markets on which these trades happen native to footium or are they secondary?

detectiveking123 commented 9 months ago

I don't think there are Footium native NFT markets at the moment. As described in this previous issue (https://github.com/sherlock-audit/2023-04-footium-judging/issues/378), the markets in which the buy/sell trades happen would be secondary markets like OpenSea, where Footium has a listing.

BoRonG0d commented 9 months ago

Even if the "Guess Frontrun" is feasible, I believe this should be low.

According to the sponsor, there is no difference in the players minted in Academy.

image

And, according to Footium's doc,

To guarantee that you keep an academy product at your club past the age of 21, you will need to sign them as a squad player – by minting him as an NFT. Failure to do so will mean that the player becomes a free agent, and any other club will be able to sign him at an auction. Once you've signed a player, you have the scope to sell and loan them out to other teams or continue playing them in your squad.

Unable to mint in one's own Academy doesn't seem like a issue, they can just sigh other palyers since they're the same.

detectiveking123 commented 9 months ago
  1. Not sure that really matters, you still lose out on the academy players. Also, NFTs are inherently distinct, so some will surely be valued more than others regardless
  2. Wouldn't you still have to pay more for the other players than you would if you just minted through your own academy?
Czar102 commented 9 months ago

If the speculation is done on the way people transact on the secondary markets, I don't think it is fair to assume that buyers wouldn't prevent minting players by the seller. The submission is based on the buyer making an incorrect assumption that the club can't lose value (change state) while listed, which (as shown in the submission) is obviously false.

I plan to reject the escalation on the above basis, unless it was mentioned in the code/docs that the clubs were intended to trade on the secondary markets without any kind of escrow, and they should preserve their state during the listing. (or any other reason why would the above reason not apply here) cc @detectiveking123

detectiveking123 commented 9 months ago

@Czar102 The fact that the secondary markets exist should be determinable through the publicly available resources provided during the contest. For example, https://footium.club/ has an OpenSea listing (and this link was provided by one of the sponsors publicly in the Discord).

Here is also a public statement from one of the sponsors that both implies secondary markets are important to Footium and should also address your other point to some extent.

Screen Shot 2024-01-01 at 8 02 49 PM

But ultimately, the basis of honeypot attacks is making users think there is value when you have the ability to just swipe it away. You are correct in that in a perfect world, the buyer would be a sophisticated smart contract that can tell when academy players for the club have already been minted, and just not buy in that case. But generally users in the real world are much less sophisticated and historically honeypotting attacks like these have been considered valid by Sherlock.

In fact, if we were to assume that users were super sophisticated and made absolutely no mistakes, no frontrunning / sandwich attack should ever be valid (at least on Ethereum), since Flashbots can just be used.

Will respect your decision either way, but in the event this issue is invalidated, I would appreciate some further clarification in the Sherlock rules about when specifically honeypot attacks are considered valid.

nevillehuang commented 9 months ago

Since arbitrum has no mempool, for a seller to front run a buyers transaction buying to mint, it would need the seller to take absolute pure speculation and guessing. For that reason, this should be invalid.

Historically, sherlock has always invalidate arbitrum front running issues (not going to find examples for now but you can look it up). Im honestly quite puzzled why the previous issue was accepted since it mentions mempool (current issue also mentions mempool), when arbitrum has no mempool at all, so the attack scenario described is incorrect

detectiveking123 commented 9 months ago

@nevillehuang Could you find at least one example to support your claim? There's an existing example (the similar issue in previous Footium audit contest) where the front-running issue was validated on Arbitrum.

0xEVom commented 9 months ago

Another judge brought up a concern that the mintable academy players for a club may not be shown in the UI. I believe it should be, as it is part of the clubs overall value (as confirmed by the sponsors) and sellers should know it before they buy. > Even if it is not, the malicious seller can publish the merkle proofs in the NFT description for example.

With regards to this and the general argument on unminted players constituting part of the club's overall value, I think this relies on an assumption that was not stated in the docs. All the sponsor's comment above says is:

The only thing that's tied to the club, is the right to mint academy players and the club's stadium (stadium is off-chain at the moment though).

Which doesn't directly imply specific unminted players are part of the value a club.

If a malicious seller publishes the Merkle proofs as part of the listing, then they are definitely relying on a mechanism that was not part of the project's original logic and the buyer is taking a risk by relying on this information.

detectiveking123 commented 9 months ago

"The only thing that's tied to the club, is the right to mint academy players" -> This seems like a direct implication to me. ERC721 tokens are non-fungible by nature, so every club will have the ability to mint different academy players (at least with a different name and face, even if there are no legendaries), which will be subjectively valued by the market.

0xEVom commented 9 months ago

Even a club with no pending unminted players (at the moment) still has a right to mint academy players tied to it (in the future).

detectiveking123 commented 9 months ago

Ah I see what you mean. Every club starts with the ability to mint 10 academy players; this is in both the publicly available and distributed Footium docs and video.

nevillehuang commented 9 months ago

@detectiveking123 I'll try to find it when I have the time, but it is not wrong to say that this issues "vulnerability details" section is completely invalid, so only the "summary" section can "possibly" make sense, which in turn would not be unfair to say no sufficient and clear attack path is highlighted. Same goes for the previous highlighted issue, which I feel is incorrectly judged, given you can't "see" transactions in the mempool when there is no mempool.

Czar102 commented 9 months ago

The fact that the secondary markets exist should be determinable through the publicly available resources provided during the contest.

Not only no, but you also can't justify the implication (secondary markets exist ==> people on these secondary markets are told to assume the current owner can't impact their valuation and that they care about whether player NFTs are minted or not – maybe they just care about the club logo?).

I would appreciate some further clarification in the Sherlock rules about when specifically honeypot attacks are considered valid.

They are not valid if you assume that users are not sophisticated. Honeypotting would be valid if the documentation directly contradicted with the current behavior (like I said in the previous message), or if the property that is broken would be obvious to be supposed to hold, which is not the case here.

Else, like in this case, honeypotting is a good informational issue. Tagging @JDansercoer to make sure the team is aware of this property of the system.

Czar102 commented 9 months ago

Result: Invalid Duplicate of #48

sherlock-admin2 commented 9 months ago

Escalations have been resolved successfully!

Escalation status: