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

Inconsistent `HorseStore.isHappyHorse` between 2 contract versions

wafflemakr

Summary

Inconsistent HorseStore.isHappyHorse between 2 contract versions

Vulnerability Details

HorseStore.isHappyHorse should return true if a horse is happy, when it was fed in within the last 24h. But, there are differences between the 2 contract versions:

  • Solidity version uses <= to compare fed time and timestamp 24 ago

  • Huff version uses lt or <= to compare 1 day in seconds and the time pass since fed and .

This creates inconsistencies when calling the function at:

  • t = fed time + 24hours - 1 sec: Huff version will return false while solidity version will return true

  • t = fed time + 24hours : Both return false

  • t = fed time + 24hours + 1 sec: Huff version will return true while solidity version will return false

The solidity version mistakingly compares horseIdToFedTimeStamp[horseId] <= block.timestamp - HORSE_HAPPY_IF_FED_WITHIN, it should be >=

The Huff version returns true if 24h < timestamp - horseFedTimestamp or timestamp == horseFedTimestamp

Even thought the logic is completely useless, as it not returning correctly if horse is happy, the values returned should be the same between both versions.

PoC

function testChecksAfterFeeding() public{
vm.prank(alice);
horseStore.mintHorse();
​
vm.warp(3 days);
​
vm.prank(alice);
horseStore.feedHorse(0);
​
assertEq(horseStore.horseIdToFedTimeStamp(0), 3 days);
assertEq(horseStore.isHappyHorse(0), true);
​
// After exactly one day - 1 sec, horse should still be happy
skip(1 days - 1);
assertEq(horseStore.isHappyHorse(0), false, "horse it happy");
​
// After exactly one day, horse should still be happy
skip(1);
assertEq(horseStore.isHappyHorse(0), false, "horse it happy");
​
// After exactly one day + 1 sec, horse should not be happy
skip(1);
assertEq(horseStore.isHappyHorse(0), true, "horse is not happy");
​
}

Impact

HIGH. Invariant broken, both contract should behave the same.

Tools Used

Manual review, Foundry.

Recommendations

Consider adjusting the timestamp check, to make sure that it only returns true if:

block.timestamp - horseIdToFedTimeStamp[horseId] <= HORSE_HAPPY_IF_FED_WITHIN (24h)
Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
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.