search

sherlock-audit / 2023-12-footium-judging

7 stars 6 forks source link

osmanozdemir1 - Some prizes can not be claimable and will be locked forever due to prizes being tied to accounts, not NFTs. #34

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

osmanozdemir1

high

Some prizes can not be claimable and will be locked forever due to prizes being tied to accounts, not NFTs.

Summary

When the prize Merkle tree is generated, the club NFTs might be held in a contract that lacks a method for claiming prizes, leading to the prizes being locked in the FootiumPrizeDistributor

Vulnerability Detail

This protocol is a blockchain gaming protocol where the users own their in-game assets, play, and earn prizes with their assets. This is the most important argument of the blockchain gaming industry: ownership of assets, and the ability to use your assets however you like.

Users play a football manager game in this protocol, the first 9 spots in a league earn prizes, and a Merkle tree is generated at the end of every season to prove who earned how much. This Merkle tree is generated based on who owns the NFT at that time and holds the lifetime value earned by the user(Club NFT owner at that time).

Here is the crucial part: Prizes are tied to accounts, not NFTs themselves.
Merkle Tree stores the total earned prizes by an account, and the smart contract tracks the total claimed prizes by an account.

    mapping(IERC20Upgradeable => mapping(address => uint256)) private totalERC20Claimed;
    mapping(address => uint256) private totalETHClaimed;

Everything is okay so far. Where is the issue?

Let's go back to the ownership argument. Players can do whatever they want with their NFTs. They can play the game, they can store it in a cold wallet, transfer to someone etc.

Now imagine this scenario:

I also would like to point out that this can not be considered a user mistake since there is no warning regarding how the players should use their club NFTs, and there is no warning that explains users must hold their NFTs in their wallet when the league ends.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-12-footium/blob/main/footium-eth-shareable/contracts/FootiumPrizeDistributor.sol#L29C1-L31C57

https://github.com/sherlock-audit/2023-12-footium/blob/main/footium-eth-shareable/contracts/FootiumPrizeDistributor.sol#L109C2-L194C6

Tool used

Manual Review

Recommendation

It is hard to recommend a solution blindly and it depends on how the protocol wants to approach this.

If the protocol wants to give flexibility to its users in terms of asset ownership, then the protocol should track prizes with NFTs themselves not with the accounts. NFTs should have an internal value of earned prizes and claimed prizes. This will probably require much more work, but it also matches the main idea of asset ownership behind blockchain gaming.

The other option is restricting users' actions. The protocol should warn users regularly to hold their tokens in their wallet, especially during the time at the end of the season when the Merkle tree is generated. The protocol should tell its users they may lose their prizes if they transfer their tokens to other vaults. In the meantime, the protocol might need to add a withdraw function to save locked tokens in case of non-compliant users.

The second option is much easier to implement but unfortunately, it contradicts the asset ownership idea which is the starting point of blockchain gaming in the first place.

nevillehuang commented 11 months ago

Based on the following reasons, I believe this issue is invalid:

  1. Merkle tree and proofs are generated off-chain assigned by the admin, as long as proofs are correctly assigned to the owner of the NFT, once the user gains back his NFT he can still claim prizes

  2. If user provide NFT as collateral, then he is not participating in the game, so he shouldn't be assigned any rewards.

osmanozdemir1 commented 11 months ago

Escalate

I would like to answer these reasonings.

  1. As far as I understand from the conversations with the sponsor, Merkle trees are generated automatically based on the NFT ownership. Prize earners are the club NFT owners at that moment. Therefore the prize earner will be assigned as the third party smart contract (vault etc) (Proofs are correctly assigned to the owner of the NFT as the judge says, but the owner at that moment is the third party contract). Getting back your NFT will not work and actually this is the exact thing I submitted in this issue. Prizes are not tied to NFTs, they are tied to accounts that holds NFT at that specific moment.

  2. Leagues take around 1 month in this protocol and first 9 places earn prizes. A user playing 25-27 days and then depositing their NFT as a collateral just for a few days doesn't mean "they didn't play the game and they don't deserve". If that club finishes in top 9 places with user's accomplishments in the first 25 days, that club's earned prize will be assigned but no one will be able to claim it.

I also would like to point out that there is no scenario where blockchain gaming and this protocol is successful but the gaming NFTs are only used in-game. They will be used for lending, borrowing, investing and even renting and many more. It is not a user mistake, it is the nature of the blockchain space. Players are not warned about they must hold their NFTs in their wallets all the time.

The impact is not only a player losing a prize, it is also the protocol losing:

  1. That deposited prize funds, since the contract does not have a withdraw method for stuck/unclaimable prizes.
  2. Users trust and engagement with the game.

With these reasoning, I strongly believe this is a valid issue due to loss of funds, viable scenario (which is not unlikely) and conditions have a reasonable chance of becoming true in the future.

There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions having a reasonable chance of becoming true in the future.

Kind regards,

sherlock-admin2 commented 11 months ago

Escalate

I would like to answer these reasonings.

  1. As far as I understand from the conversations with the sponsor, Merkle trees are generated automatically based on the NFT ownership. Prize earners are the club NFT owners at that moment. Therefore the prize earner will be assigned as the third party smart contract (vault etc) (Proofs are correctly assigned to the owner of the NFT as the judge says, but the owner at that moment is the third party contract). Getting back your NFT will not work and actually this is the exact thing I submitted in this issue. Prizes are not tied to NFTs, they are tied to accounts that holds NFT at that specific moment.

  2. Leagues take around 1 month in this protocol and first 9 places earn prizes. A user playing 25-27 days and then depositing their NFT as a collateral just for a few days doesn't mean "they didn't play the game and they don't deserve". If that club finishes in top 9 places with user's accomplishments in the first 25 days, that club's earned prize will be assigned but no one will be able to claim it.

I also would like to point out that there is no scenario where blockchain gaming and this protocol is successful but the gaming NFTs are only used in-game. They will be used for lending, borrowing, investing and even renting and many more. It is not a user mistake, it is the nature of the blockchain space. Players are not warned about they must hold their NFTs in their wallets all the time.

The impact is not only a player losing a prize, it is also the protocol losing:

  1. That deposited prize funds, since the contract does not have a withdraw method for stuck/unclaimable prizes.
  2. Users trust and engagement with the game.

With these reasoning, I strongly believe this is a valid issue due to loss of funds, viable scenario (which is not unlikely) and conditions have a reasonable chance of becoming true in the future.

There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions having a reasonable chance of becoming true in the future.

Kind regards,

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 11 months ago

@osmanozdemir1

TLDR, you are highlighting a potential timing issue about the merkle-proof generation that is an off-chain design opinion out of scope for this contest.

osmanozdemir1 commented 11 months ago

@nevillehuang

Long story short: The issue is not about incorrect Merkle tree generation or timing of it. We already know it is generated every time when a league ends. There is loss of funds regardless of the off-chain design opinion. Solutions are either changing both on-chain claimed prize tracking logic and off-chain tree generation, or implementing an on-chain withdraw method.

nevillehuang commented 11 months ago

@osmanozdemir1

To me, what you are describing requires out of scope understanding of the off-chain merkle proof generation not known to any watson.

We already know it is generated every time when a league ends

Is this publicly available info? If not it is fair for watsons to assume the admin is trusted to generate correct merkle proofs. Since the solution can be implemented off-chain, then I still think this should be invalid, given it is out of scope. I will loop in sponsor @JDansercoer @logiclogue and head of judging @Czar102 to verify my claims.

osmanozdemir1 commented 11 months ago

@nevillehuang Thanks for the reply.

We already know it is generated every time when a league ends

Just to clarify, the reason I'm saying that is this logic. For this to happen, Merkle tree must be updated every time a league ends by adding latest prizes. Maybe I should've said "we must assume" instead of "we already know".

Thanks again and I'll wait your responses. Kind regards,

nevillehuang commented 11 months ago

@osmanozdemir1 Based on your initial comment, I still maintain my stance as invalid and will wait for sponsor and head of judging to comment

  1. The following statement you made is an out-of-scope description of how the off-chain merkle tree/proof generation works, nothing in relation to the contracts in scope. If the following is publicly known knowledge, I would have judged it otherwise.

Merkle trees are generated automatically based on the NFT ownership

  1. Because of the above, it is completely fair that watsons think the trusted admins are generating merkle proofs manually for users, and will not make mistakes even when NFTs are used for other purposes (i.e. incorrectly generating merkle proof for an unknown smart contract address), as they would have known the club owners upon initial minting or if club is transferred during a sale.
Czar102 commented 11 months ago

From my understanding, "will not make mistakes [in merkle proofs]" means that the admins will choose an address that is able to claim the reward.

I fully agree with the Lead Judge @nevillehuang. I have nothing more to add to his argument. Also, this is an opportunity loss or an integration issue. Hence, the report is out of scope on this basis, too.

Planning to reject the escalation and leave the issue as is.

Czar102 commented 11 months ago

Result: Invalid Unique

sherlock-admin2 commented 11 months ago

Escalations have been resolved successfully!

Escalation status: