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

Some mappings are implemented without storage slot, potentially causing hash collision

glitchr

Description

Both the horseId => fedTimestamp and (owner, operator) => approval for all mappings are used without a storage slot.

#define macro FEED_HORSE() = takes (0) returns (0) {
timestamp // [timestamp]
0x04 calldataload // [horseId, timestamp]
STORE_ELEMENT(0x00) // []
#define macro SET_APPROVAL_FOR_ALL() = takes (0) returns (0) {
0x24 calldataload // [approved]
0x04 calldataload // [operator, approved]
caller // [msg.sender, operator, approved]
STORE_ELEMENT_FROM_KEYS(0x00) // []

Impact

Only the value of the keys is hashed to define the storage slot location and this could result in a hash collision, in particular if more features are added to the contract.

Proof on concept

If two mappings contain the same key (for example 0), then they will both use the same storage location (keccak256(0)) for the same key.

If a storage slot is defined for each mapping, then the storage pointer will be added to the key hashes, which will guarantee that both mappings will use different storage pointers for their keys, thus avoiding collision.

See Hashmap.huff::STORE_ELEMENT and Hashmap.huff::GET_SLOT_FROM_KEY.

Recommended mitigation

Define a storage slot for all mappings and use

  • STORE_ELEMENT_FROM_KEYS for the horseId => fedTimestamp mapping.

  • STORE_ELEMENT_FROM_KEYS_2D for the (owner, operator) => approval for all mapping.

Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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