mstpr-brainbot - Clubs can mint +1 players more than maxGenerationId

[sherlock-admin]

mstpr-brainbot

high

Clubs can mint +1 players more than maxGenerationId

Summary

As stated in doc here

Initially clubs should be able to mint 10 players. However, clubs can always mint +1 players more than maxGenerationId

Vulnerability Detail

An off-by-one error is existed in academy contract when checking the generation ID while minting players. The code currently allows clubs to mint up to maxGenerationId + 1 players, which is inconsistent with the documentation. If clubs send generation IDs in the following sequence: 0-1-2-3-4-5-6-7-8-9-10, all of these numbers will be valid, and the club will be able to mint maxGenerationId + 1 players instead of the intended maxGenerationId

Impact

Since this is not intended behaviour according to the protocol docs, I'll label it as high.

Code Snippet

https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L186-L188
here code checks whether the generation id is higher than maxGeneration or not, and it can be equal to maxGeneration.

Tool used

Manual Review

Recommendation

Use generationId >= _maxGenerationId or do not count the "0" index

[logiclogue]

Marked as 'Will Fix'. I believe the solution here is instead to update the documentation to say "At game launch this value will be 9". Because the documentation as far as I'm aware is correct in that it says maxGenerationId is the maximum generation ID, it's just that it does not represent the total players per cohort, which was the assumption with the default value being 10.

[dugdaniels]

Escalate for 10 USDC

This should be considered Low/Informational.

The variable is named maxGenerationId. As the name implies (and as noted in the docs), it is the maximum integer ID that can be used to mint. A maxGenerationID of 10 therefore means that you can mint a player with an ID of 10. The sponsor confirmed that this is working as intended and that only the documentation needs to be clarified.

Furthermore, to imply that a maxGenerationId of 10 should actually enforce a max id of 9 is illogical.

Ultimately, it's a configuration variable and the sponsor is updating the documentation to reflect the value that will be set to at launch. A documentation update should only be considered as informational. There is no exploit here.

[sherlock-admin]

Escalate for 10 USDC

This should be considered Low/Informational.

The variable is named maxGenerationId. As the name implies (and as noted in the docs), it is the maximum integer ID that can be used to mint. A maxGenerationID of 10 therefore means that you can mint a player with an ID of 10. The sponsor confirmed that this is working as intended and that only the documentation needs to be clarified.

Furthermore, to imply that a maxGenerationId of 10 should actually enforce a max id of 9 is illogical.

Ultimately, it's a configuration variable and the sponsor is updating the documentation to reflect the value that will be set to at launch. A documentation update should only be considered as informational. There is no exploit here.

You've created a valid escalation for 10 USDC!

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.

[NishithPat]

Escalate for 10 USDC
Disagree with the sponsor's comment saying that the documentation is right and that maxGenerationId is "the maximum generation ID, it's just that it does not represent the total players per cohort" or "it is the maximum integer ID that can be used to mint".

The docs say that maxGenerationId is "the number of players that can be minted per cohort". Check the definition of maxGenerationId in the doc.

Then going by the docs, if maxGenerationId is the number of players that can be minted per cohort, then the contract does indeed allow maxGenerationId + 1 player to be minted. In that case, the issue should be valid.

[sherlock-admin]

Escalate for 10 USDC
Disagree with the sponsor's comment saying that the documentation is right and that maxGenerationId is "the maximum generation ID, it's just that it does not represent the total players per cohort" or "it is the maximum integer ID that can be used to mint".

The docs say that maxGenerationId is "the number of players that can be minted per cohort". Check the definition of maxGenerationId in the doc.

Then going by the docs, if maxGenerationId is the number of players that can be minted per cohort, then the contract does indeed allow maxGenerationId + 1 player to be minted. In that case, the issue should be valid.

You've created a valid escalation for 10 USDC!

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.

[logiclogue]

Sponsor here, agreed with @NishithPat . My original statement is incorrect, the documentation should be updated to remove the statement "the number of players that can be minted per cohort". Their suggestion is correct

[ctf-sec]

Based on the comments above, NishithPat escalation is valid

[hrishibhat]

Escalation accepted

Valid medium
Accepting @NishithPat's escalation
Given that the issue correctly identifies the flaw in the code as against what the documentation says it's supposed to do, this is a valid issue.

[sherlock-admin]

Escalation accepted

Valid medium
Accepting @NishithPat's escalation
Given that the issue correctly identifies the flaw in the code as against what the documentation says it's supposed to do, this is a valid issue.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.