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

Incorrect control flow, input verifications and implementations

ceseshi

[L-1] Incorrect control flow makes the contract return incorrect values for undefined functions

Summary

The control flow in MAIN macro is incorrectly implemented, so any function called whose selector is not defined will be passed to GET_TOTAL_SUPPLY().

Vulnerability Details

If no defined function matches the function selector called, it will end up in totalSupply:

dup1 __FUNC_SIG(getApproved) eq getApproved jumpi
dup1 __FUNC_SIG(isApprovedForAll) eq isApprovedForAll jumpi
​
dup1 __FUNC_SIG(balanceOf) eq balanceOf jumpi
dup1 __FUNC_SIG(ownerOf)eq ownerOf jumpi
@> totalSupply:
GET_TOTAL_SUPPLY()
feedHorse:
FEED_HORSE()
isHappyHorse:
IS_HAPPY_HORSE()

Impact

Calling any undefined function will always return totalSupply, which may cause other protocols to receive confusing information from the NFT collection.

Proof of Concept

This test mints one token and gets the token index, which should be zero but will be equal to totalSupply.

function testUnimplementedFunction() public {
vm.prank(user);
horseStore.mintHorse();
uint256 token0index = horseStore.tokenByIndex(0);
assertEq(token0index, horseStore.totalSupply());
}

Run Huff test

forge test --mc HorseStoreHuff --mt testUnimplementedFunction

The test passes confirming that tokenByIndex() is returning totalSupply()

Running 1 test for test/HorseStoreHuff.t.sol:HorseStoreHuff
[PASS] testUnimplementedFunction() (gas: 62709)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.63s

Recommended Mitigation

Revert if the called function is not implemented

dup1 __FUNC_SIG(balanceOf) eq balanceOf jumpi
dup1 __FUNC_SIG(ownerOf)eq ownerOf jumpi
​
+ // no match
+ 0x00 dup1 revert
+
totalSupply:
GET_TOTAL_SUPPLY()

[I-1] isHappyHorse() does not verify horse id, so the contract will return incorrect information

Summary

isHappyHorse() does not verify the horseId, so it will return false for any non-existent horse id.

Impact

External protocols can receive incorrect information from the NFT collection.

Proof of Concept

This test feeds a horse with an arbitrary Id.

function testAnyHorseIdCanBeChecked() public {
vm.warp(horseStore.HORSE_HAPPY_IF_FED_WITHIN());
assertEq(horseStore.isHappyHorse(1337), false);
}

Run Solidity and Huff tests

forge test --mt testCanFeedNonexistentHorse

Both test pass, even though they receive a non-existent horse Id.

Running 1 test for test/HorseStoreSolidity.t.sol:HorseStoreSolidity
[PASS] testAnyHorseIdCanBeChecked() (gas: 27664)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.87ms
​
Running 1 test for test/HorseStoreHuff.t.sol:HorseStoreHuff
[PASS] testAnyHorseIdCanBeChecked() (gas: 27457)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.52s

Recommended Mitigation

Verify that the horse exists, and throw a custom error if it does not.

Corrections for Solidity

mapping(uint256 id => uint256 lastFedTimeStamp) public horseIdToFedTimeStamp;
​
+ error HorseDoesNotExist();
+
constructor() ERC721(NFT_NAME, NFT_SYMBOL) {}
function isHappyHorse(uint256 horseId) external view returns (bool) {
+ if (_ownerOf(horseId) == address(0)) {
+ revert HorseDoesNotExist();
+ }
+
if (horseIdToFedTimeStamp[horseId] <= block.timestamp - HORSE_HAPPY_IF_FED_WITHIN) {

Corrections for Huff

#define macro IS_HAPPY_HORSE() = takes (0) returns (0) {
+ 0x04 calldataload // [tokenId]
+ [OWNER_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [owner]
+ // revert if owner is zero address/not minted
+ continue jumpi
+ NOT_MINTED(0x00)
+ continue:
​
0x04 calldataload // [horseId]

[I-2] (Huff) Implementation of ERC721 standard is incomplete, so some functions of the standard interface are not usable

Summary

The Huff implementation lacks the safeTransferFrom(), tokenByIndex() and tokenOfOwnerByIndex() functions from the ERC721Enumerable interface, so they cannot be used.

Impact

External protocols may have issues interacting with the NFT collection.

Proof of Concept

The Huff implementation does not include the standard functions.

Recommended Mitigation

Add Huff implementations for safeTransferFrom(), tokenByIndex() and tokenOfOwnerByIndex(), or explicitly state that this collection doesn't support them.

[I-3] Incorrect call to MINT_HORSE()

Summary

The MAIN macro has an incorrect call to MINT_HORSE() at the end, which is never called, but should not exist there.

Vulnerability Details

balanceOf:
BALANCE_OF()
ownerOf:
OWNER_OF()
@> MINT_HORSE()
}

Impact

No impact on the protocol.

Recommended Mitigation

Remove the incorrect call.

ownerOf:
OWNER_OF()
- MINT_HORSE()
}
Updates

Lead Judging Commences

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

MAIN() macro is not properly implemented

Any call data sent to the contract that doesn't contain a function selector will randomly mint a horse.

Components of ERC721 not properly (or at all) implemented in HUFF

Support

FAQs

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