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

`HuffStore.huff::IS_HAPPY_HORSE` returns the opposite of the expected result

n0kto

Description

The HuffStore.huff::IS_HAPPY_HORSE macro is intended to check if a horse has been fed after yesterday. However, due to the use of the lt opcode, it returns true when the horse is starving and the opposite when it is fed. There is an exception with the eq opcode, where it returns true when FEED_HORSE is called in the same transaction as IS_HAPPY_HORSE (in that order), such as in a smart contract.

#define macro IS_HAPPY_HORSE() = takes (0) returns (0) {
0x04 calldataload // [horseId]
LOAD_ELEMENT(0x00) // [horseFedTimestamp]
timestamp // [timestamp, horseFedTimestamp]
dup2 dup2 // [timestamp, horseFedTimestamp, timestamp, horseFedTimestamp]
sub // [timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
[HORSE_HAPPY_IF_FED_WITHIN_CONST] // [HORSE_HAPPY_IF_FED_WITHIN, timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
@> lt // [HORSE_HAPPY_IF_FED_WITHIN < timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
start_return_true jumpi // [timestamp, horseFedTimestamp]
eq // [timestamp == horseFedTimestamp]
start_return
jump
​
start_return_true:
0x01
​
start_return:
// Store value in memory.
0x00 mstore
​
// Return value
0x20 0x00 return
}

Impact

This inconsistency breaks the logic and the protocol.

Proof of Concept

Foundry PoC
function testHappyWhenStarvingAndAngryWhenFed() public {
vm.warp(2 days);
uint id = horseStore.totalSupply();
​
vm.startPrank(user);
horseStore.mintHorse();
horseStore.feedHorse(id);
skip(10);
bool isHappy = horseStore.isHappyHorse(id);
​
// will fail because the horse is not happy when it is fed
assertTrue(isHappy);
vm.warp(4 days);
bool isNotHappy = horseStore.isHappyHorse(id);
​
// will fail because the horse is happy when it is starving
assertFalse(isNotHappy);
}

Recommended Mitigation

To address this issue, change the lt opcode to gt opcode. Additionally, you can remove the eq and dup2 dup2 part, as there is no case where it is reachable now. To return false, keep the jump and add 0x00 before. Here is a suggested solution:

#define macro IS_HAPPY_HORSE() = takes (0) returns (0) {
0x04 calldataload // [horseId]
LOAD_ELEMENT(0x00) // [horseFedTimestamp]
timestamp // [timestamp, horseFedTimestamp]
- dup2 dup2 // [timestamp, horseFedTimestamp, timestamp, horseFedTimestamp]
- sub // [timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
+ sub // [timestamp - horseFedTimestamp]
- [HORSE_HAPPY_IF_FED_WITHIN_CONST] // [HORSE_HAPPY_IF_FED_WITHIN, timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
+ [HORSE_HAPPY_IF_FED_WITHIN_CONST] // [HORSE_HAPPY_IF_FED_WITHIN, timestamp - horseFedTimestamp]
- lt // [HORSE_HAPPY_IF_FED_WITHIN < timestamp - horseFedTimestamp, timestamp, horseFedTimestamp]
+ gt // [HORSE_HAPPY_IF_FED_WITHIN > timestamp - horseFedTimestamp]
start_return_true jumpi // [timestamp, horseFedTimestamp]
- eq // [timestamp == horseFedTimestamp]
+ 0x00
start_return
jump
​
start_return_true:
0x01
​
start_return:
// Store value in memory.
0x00 mstore
​
// Return value
0x20 0x00 return
}
Updates

Lead Judging Commences

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

Wrong comparison in IS_HAPPY_HORSE()

Support

FAQs

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