juancito - Increasing `_maxGenerationId` allows extra minting of academy players on previous seasons
sherlock-admin opened this issue · 5 comments
juancito
medium
Increasing _maxGenerationId allows extra minting of academy players on previous seasons
Summary
_maxGenerationId is defined in the Footium Academy docs as:
maxGenerationId: The maximum integer generationId that can be used to mint players. This is the number of players that can be minted per cohort.
Due to a lack of checks, if maxGenerationId is increased, it is possible to mint extra players.
Vulnerability Detail
_validateMintingParams allows minting on previous seasons:
if (SeasonID.unwrap(seasonId) <= 0) {
revert InvalidSeasonId(seasonId);
}
if (SeasonID.unwrap(seasonId) > currentSeasonId) {
revert PlayerTooYoung(seasonId);
}mintPlayers doesn't enforce a limit of minted players per season either:
for (uint256 i = 0; i < generationIds.length; ) {
generationId = generationIds[i];
if (generationId > _maxGenerationId) {
revert GenerationIDTooHigh(generationId, _maxGenerationId);
}
if (redeemed[seasonId][clubId][generationId]) {
revert PlayerAlreadyRedeemed(generationId);
}
redeemed[seasonId][clubId][generationId] = true;
uint256 playerId = _footiumPlayer.safeMint(
_footiumClub.clubToEscrow(clubId)
);
emit AcademyPlayerMinted(seasonId, clubId, generationId, playerId);
unchecked {
i++;
}
}Those two factors combined are the reason for the bug.
Add this test to FootiumAcademy.test.ts to prove how extra players can be minted on previous seasons if _maxGenerationId is increased (it doesn't only affect the current season).
it.only("mints extra players for past seasons when maxGenerationId is increased", async () => {
// Set season 1
await expect(academy.changeCurrentSeasonId(1))
.to.emit(academy, "ChangedCurrentSeasonId")
.withArgs(1);
const maxGenerationId = 3; // @audit max 3 players
await expect(academy.changeMaxGenerationId(maxGenerationId))
.to.emit(academy, "ChangedMaxGenerationId")
.withArgs(maxGenerationId);
//const token = [clubId = 7, division = 4]
const clubDivProof = clubDivsTree.getHexProof(
hashClubDivisionInputs([7, 4])
);
const divisionTier = 4;
const generationIds = [0, 1, 2]; // @audit mint 3 players
const ethAmount = ethers.BigNumber.from(divisionFees[0]).mul(
generationIds.length
);
await expect(
academy
.connect(addr1)
.mintPlayers(
1,
7,
divisionTier,
generationIds,
clubDivProof,
{
value: ethAmount,
}
)
).to.not.reverted;
// Advance to season 2
await expect(academy.changeCurrentSeasonId(2))
.to.emit(academy, "ChangedCurrentSeasonId")
.withArgs(2);
// Increase the generation length to 6
// It should only affect the current generation, but it affect the past ones in fact as well
const newMaxGenerationId = 6; // @audit max 6 players
await expect(academy.changeMaxGenerationId(newMaxGenerationId))
.to.emit(academy, "ChangedMaxGenerationId")
.withArgs(newMaxGenerationId);
const extraGenerationIds = [3, 4, 5]; // @audit mint 3 more players
await expect(
academy
.connect(addr1)
.mintPlayers(
1, // @audit mint for the previous season
7,
divisionTier,
extraGenerationIds, // @audit extra players
clubDivProof,
{
value: ethAmount,
}
)
).to.not.reverted;
// It was possible to mint three extra players on the previous season
});Impact
Extra academy players can be minted bypassing the expected limitations.
Code Snippet
Tool used
Manual Review
Recommendation
Define a _maxGenerationId limit per season and check it accordingly.
Duplicate of #319
Escalate for 10 USDC
This issue is a duplicate of #319, not of #152.
I'd also like to request to consider this report as the main one as it provides a coded POC demonstrating its validity, as well as addressing the impact and validity correctly as the former primary issue.
Duplicate of #319, not #152
#319 issue: changeMaxGenerationId allows to mint tokens from older generations retroactively
This issue: Increasing _maxGenerationId allows extra minting of academy players on previous seasons
On the other hand #152: Clubs can mint +1 players more than maxGenerationId
I have already created an issue for that one on #273: One extra academy player can be minted per season due to mischeck in mintPlayers
Same impact
#319 issue: Minting of X tokens where X is the number of old seasons
This issue title: ...allows extra minting of academy players on previous seasons
This issue impact: Extra academy players can be minted bypassing the expected limitations.
Same root cause
#319 issue:
changeMaxGenerationId is meant to allow more minting for the current generation, however, because of the fact that merkleProofs do not validate for the current season, whenever the limit will be raised, the limit will allow to mint players from older seasons
This issue shows it with the coded POC:
// Increase the generation length to 6
// It should only affect the current generation, but it affect the past ones in fact as well
const newMaxGenerationId = 6; // @audit max 6 players
await expect(academy.changeMaxGenerationId(newMaxGenerationId))
.to.emit(academy, "ChangedMaxGenerationId")
.withArgs(newMaxGenerationId);And also provides further explanation of the root cause of this with:
_validateMintingParams allows minting on previous seasons
mintPlayers doesn't enforce a limit of minted players per season either:
Same mitigation
#319 issue:
Enforce minting exclusively for the current season or change validation to validate the generations and seasons being minted
This issue:
Define a _maxGenerationId limit per season and check it accordingly.
Escalate for 10 USDC
This issue is a duplicate of #319, not of #152.
I'd also like to request to consider this report as the main one as it provides a coded POC demonstrating its validity, as well as addressing the impact and validity correctly as the former primary issue.
Duplicate of #319, not #152
#319 issue:
changeMaxGenerationId allows to mint tokens from older generations retroactively
This issue:Increasing _maxGenerationId allows extra minting of academy players on previous seasonsOn the other hand #152:
Clubs can mint +1 players more than maxGenerationId
I have already created an issue for that one on #273:One extra academy player can be minted per season due to mischeck in mintPlayersSame impact
#319 issue:
Minting of X tokens where X is the number of old seasons
This issue title:...allows extra minting of academy players on previous seasons
This issue impact:Extra academy players can be minted bypassing the expected limitations.Same root cause
#319 issue:
changeMaxGenerationId is meant to allow more minting for the current generation, however, because of the fact that merkleProofs do not validate for the current season, whenever the limit will be raised, the limit will allow to mint players from older seasons
This issue shows it with the coded POC:
// Increase the generation length to 6 // It should only affect the current generation, but it affect the past ones in fact as well const newMaxGenerationId = 6; // @audit max 6 players await expect(academy.changeMaxGenerationId(newMaxGenerationId)) .to.emit(academy, "ChangedMaxGenerationId") .withArgs(newMaxGenerationId);And also provides further explanation of the root cause of this with:
_validateMintingParams allows minting on previous seasons
mintPlayers doesn't enforce a limit of minted players per season either:Same mitigation
#319 issue:
Enforce minting exclusively for the current season or change validation to validate the generations and seasons being minted
This issue:
Define a _maxGenerationId limit per season and check it accordingly.
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.
Escalation accepted
Valid duplicate of #319
Escalation accepted
Valid duplicate #319
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.