First Flight #7: Horse Store

Summary :

The code has a potential vulnerability related to the storage and retrieval of last fed timestamp for each horse. The use of mapping(uint256 id => uint256 lastFedTimeStamp) may lead to unexpected behaviour and a potential exploit. The type of bug described in the provided analysis is a timestamp manipulation vulnerability. This occurs when the timestamp is used as a critical factor in a smart contract's logic, and an attacker can manipulate it within the same block to deceive the contract's functionality. In this specific case, the vulnerability allows a malicious user to manipulate the last fed timestamp of a horse within the same block, potentially making the horse always appear happy regardless of when it was actually fed.

Vulnerability Details :

The vulnerability lies in the way the last fed timestamp is stored and checked in the isHappyHorse function. The code uses block.timestamp for both setting and checking the last fed timestamp. This can lead to an issue where the timestamp is updated in the same block, causing the isHappyHorse function to incorrectly evaluate the happiness of the horse.

                     function feedHorse(uint256 horseId) external {
                              horseIdToFedTimeStamp[horseId] = block.timestamp;
                            }
                     function isHappyHorse(uint256 horseId) external view returns (bool) {
                              if (horseIdToFedTimeStamp[horseId] <= block.timestamp - HORSE_HAPPY_IF_FED_WITHIN) {
                              return false;
                            }
                              return true;
                            }

Impact :

The impact of this vulnerability is that a malicious user could manipulate the last fed timestamp within the same block, making the horse always appear happy even if it has not been fed within the specified time. This undermines the intended functionality of the isHappyHorse function.

Proof of Concept :

Suppose a malicious user calls the feedHorse function and updates the timestamp within the same block:

                     // Assume HORSE_HAPPY_IF_FED_WITHIN is set to 1 day
                              uint256 horseId = 0; // Assuming there is a horse with ID 0
                     // Malicious user updates the timestamp within the same block
                              HorseStore.feedHorse{gas: 2000000}(horseId);
                     // Check if the horse is happy immediately after
                              bool isHappy = HorseStore.isHappyHorse{gas: 2000000}(horseId);
                     // isHappy will incorrectly return true, even though not enough time has passed

Tools Used :

Manual code review.

Recommendations :

To mitigate this vulnerability, it is recommended to use block.number instead of block.timestamp for storing and checking the last fed timestamp. This ensures that the timestamp is unique for each block and prevents manipulation within the same block.

Modify the feedHorse function as follows:

                     function feedHorse(uint256 horseId) external {
                              horseIdToFedTimeStamp[horseId] = block.number;
                             }

And update the isHappyHorse function accordingly:

                     function isHappyHorse(uint256 horseId) external view returns (bool) {
                              if (horseIdToFedTimeStamp[horseId] <= block.number - (HORSE_HAPPY_IF_FED_WITHIN / 15 seconds)) {
                     return false;
                              }
                     return true;
                      }

Note:

1. The division by 15 seconds is added to roughly convert the block numbers to timestamps for comparison. The exact conversion may depend on the block time of the Ethereum network.