First Flight #7: Horse Store
First Flight #7
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Second user cannot mint horses as TotalSupply not updated

jerseyjoewalcott

Summary

In the huff version, only the first user can mint horses, the second user onwards is unable to mint horses as TOTAL_SUPPLY is not updated.

Vulnerability Details

At no point is the TOTAL_SUPPLY updated, and when mint_horse() is called which uses total_supply to mint the horse for the next tokenId, will throw an error as the TOTAL_SUPPLY is still 0.

Here is a POC to test more than user minting the nft where the solidity works fine and the huff will revert as it is ALREADY_MINTED:

function testMintingHorseAssignsOwnerPOC(address randomOwner, address randomOwner2) public {
vm.assume(randomOwner != address(0));
vm.assume(!_isContract(randomOwner));
​
vm.assume(randomOwner2 != address(0));
vm.assume(randomOwner2 != randomOwner);
vm.assume(!_isContract(randomOwner2));
​
uint256 horseId = horseStore.totalSupply();
console2.log(horseId);
vm.prank(randomOwner);
horseStore.mintHorse();
assertEq(horseStore.ownerOf(horseId), randomOwner);
​
uint256 horseTotalSupplyAfter = horseStore.totalSupply();
console2.log(horseStore.ownerOf(horseId));
uint256 horseId2 = horseStore.totalSupply();
vm.prank(randomOwner2);
horseStore.mintHorse();
assertEq(horseStore.ownerOf(horseId2), randomOwner2);
​
}

Impact

Protocol cannot function properly as it is only possible to mint NFT making all the equestrian lovers unhappy. High as a core functionality of protocol does not work as intended.

Tools Used

Foundry

Recommendations

https://github.com/Cyfrin/2024-01-horse-store/blob/8255173c9340c12501881c9ecdd4175ff7350f5d/src/HorseStore.huff#L73-L78

add sload// [totalSupply] to line 75.

#define macro MINT_HORSE() = takes (0) returns (0) {
[TOTAL_SUPPLY] // [TOTAL_SUPPLY]
sload// [totalSupply]
caller // [msg.sender, TOTAL_SUPPLY]
_MINT() // []
stop // []
}

and

https://github.com/Cyfrin/2024-01-horse-store/blob/8255173c9340c12501881c9ecdd4175ff7350f5d/src/HorseStore.huff#L318-L352

add

// Increment total supply
[TOTAL_SUPPLY] sload // Load the current total supply
0x01 add // Increment it by 1
[TOTAL_SUPPLY] sstore // Store the updated value back

to the mint function like so:

#define macro _MINT() = takes (2) returns (0) {
// Input stack: // [to, tokenId]
// Output stack: // []
​
// Check that the recipient is valid
dup1 iszero invalid_recipient jumpi // [to, tokenId]
​
// Create the minting params
0x00 dup3 // [tokenId, from (0x00), to, tokenId]
​
// Check token ownership
[OWNER_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [owner, from (0x00), to, tokenId]
unauthorized jumpi
​
// Increment total supply
[TOTAL_SUPPLY] sload // Load the current total supply
0x01 add // Increment it by 1
[TOTAL_SUPPLY] sstore // Store the updated value back
​
// Give tokens to the recipient.
TRANSFER_GIVE_TO() // [from (0x00), to, tokenId]
​
// Emit the transfer event.
__EVENT_HASH(Transfer) // [sig, from (0x00), to, tokenId]
0x00 0x00 log4 // []
​
// Continue Executing
cont jump
​
invalid_recipient:
INVALID_RECIPIENT(0x00)
​
unauthorized:
ALREADY_MINTED(0x00)
​
cont:
}

Sorry I'm not sure whats the correct markdown for this. Also I'm not sure if thats the correct implementation to update TOTAL_SUPPLY

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Failure to increment total supply on mint

Failure to properly load the totalSupply in Huff

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.