Weather Witness
First Flight #40
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Request State Not Cleaned Up, Leading to Storage Bloat and Potential Replay Issues

albert

Root + Impact

Description

The contract uses mappings s_funcReqIdToUserMintReq and s_funcReqIdToMintFunctionReqResponse to store temporary state related to minting requests during the Chainlink Functions call lifecycle. After a mint request is processed (either successfully minted or failed), the entries in these mappings for the corresponding requestId are not deleted.
This leads to:

  1. Storage Bloat: These mappings will grow indefinitely with every mint attempt, consuming storage and potentially increasing gas costs for other operations over time if the chain charges for state size or if these mappings were ever iterated (though they are not currently).

  2. Potential Replay/Confusion (Minor Risk): While s_tokenCounter prevents re-minting the same tokenId in a simple replay, stale data in these mappings could theoretically be misused or cause confusion if fulfillMintRequest were called again with an old requestId under specific, albeit unlikely, edge conditions where other checks might pass or fail unexpectedly. The primary concern is storage bloat.

// Root cause in the codebase with @> marks to highlight the relevant section
// File: src/WeatherNft.sol
​
function requestMintWeatherNFT(
// ...
) external payable returns (bytes32 _reqId) {
// ...
// @> s_funcReqIdToUserMintReq[_reqId] = UserMintRequest({ /* ... */ }); // Entry created
// ...
}
​
function fulfillRequest( // Oracle callback
bytes32 requestId,
bytes memory response,
bytes memory err
) internal override {
if (s_funcReqIdToUserMintReq[requestId].user != address(0)) { // Is a mint request
// @> s_funcReqIdToMintFunctionReqResponse[requestId] = MintFunctionReqResponse({ // Entry created/updated
response: response,
err: err
});
}
else if (s_funcReqIdToTokenIdUpdate[requestId] > 0) { // Is an update request
_fulfillWeatherUpdate(requestId, response, err);
}
// @> No deletion of s_funcReqIdToUserMintReq[requestId] or s_funcReqIdToMintFunctionReqResponse[requestId] here
}
​
function fulfillMintRequest(bytes32 requestId) external {
// ...
// Reads from s_funcReqIdToUserMintReq[requestId] and s_funcReqIdToMintFunctionReqResponse[requestId]
// ...
// Successful minting logic...
// ...
// @> // At the end of this function, s_funcReqIdToUserMintReq[requestId] and
// @> // s_funcReqIdToMintFunctionReqResponse[requestId] are NOT deleted.
}
​
// Similarly, s_funcReqIdToTokenIdUpdate[requestId] is populated in performUpkeep
// and used in _fulfillWeatherUpdate (via fulfillRequest).
// It's also not explicitly cleaned up after the update is fulfilled.
function performUpkeep(bytes calldata performData) external override {
// ...
// @> s_funcReqIdToTokenIdUpdate[_reqId] = _tokenId; // Entry created for update requests
}
​
function _fulfillWeatherUpdate(bytes32 requestId, bytes memory response, bytes memory err) internal {
// ... reads s_funcReqIdToTokenIdUpdate[requestId]
// @> // s_funcReqIdToTokenIdUpdate[requestId] is NOT deleted here.
}
​

Risk

Likelihood: High

  • The absence of delete operations for these map entries is a consistent pattern for all requests.

Impact: Low

  • Storage Bloat: This is the primary impact. While individual map entries are small, over a large number of mints/updates, this can accumulate. Gas refunds for deleting storage exist, which are being missed.

  • Increased State Size: Makes the contract state larger than necessary.

  • Minor Replay/Error Risk: Extremely low chance of causing issues, but uncleared state is generally an anti-pattern. For example, if fulfillMintRequest was called, reverted mid-way after some checks but before s_tokenCounter was used to prevent actual re-mint, a subsequent call might operate on stale data if not for other protections.

Proof of Concept

  1. User A calls requestMintWeatherNFT. An entry for reqIdA is created in s_funcReqIdToUserMintReq.

  2. Oracle responds for reqIdA. An entry for reqIdA is created/updated in s_funcReqIdToMintFunctionReqResponse.

  3. User A calls fulfillMintRequest(reqIdA). The NFT is minted.

  4. The entries for reqIdA in s_funcReqIdToUserMintReq and s_funcReqIdToMintFunctionReqResponse remain in storage.

  5. Repeat for User B, User C, etc. Each successful or failed mint attempt leaves data behind.

  6. Similarly, each performUpkeep cycle that sends a Functions request will create an entry in s_funcReqIdToTokenIdUpdate which is not cleared after the update is processed.

Recommended Mitigation

Delete the entries from the state mappings once they are no longer needed.

  • In fulfillMintRequest, after the request is fully processed (minted or handled as an error), delete s_funcReqIdToUserMintReq[requestId] and s_funcReqIdToMintFunctionReqResponse[requestId].

  • In _fulfillWeatherUpdate, after the update is processed, delete s_funcReqIdToTokenIdUpdate[requestId].

// File: src/WeatherNft.sol
​
function fulfillMintRequest(bytes32 requestId) external {
// ... (existing logic, including UserMintRequest retrieval and checks) ...
// ... (minting logic) ...
// ... (keeper registration logic) ...
// ... (s_weatherNftInfo update) ...
​
+ // Clean up state for the completed mint request
+ delete s_funcReqIdToUserMintReq[requestId];
+ delete s_funcReqIdToMintFunctionReqResponse[requestId];
}
​
function _fulfillWeatherUpdate(bytes32 requestId, bytes memory response, bytes memory err) internal {
if (response.length == 0 || err.length > 0) {
+ // Clean up even if there was an error, as the request is processed
+ delete s_funcReqIdToTokenIdUpdate[requestId];
return;
}
​
uint256 tokenId = s_funcReqIdToTokenIdUpdate[requestId];
// If tokenId is 0 from a cleared mapping due to prior error, this check prevents further action.
// This indicates the requestID might have been processed or was invalid.
require(tokenId != 0, "WeatherNft__InvalidTokenIdForUpdate");
​
uint8 weather = abi.decode(response, (uint8));
s_weatherNftInfo[tokenId].lastFulfilledAt = block.timestamp;
s_tokenIdToWeather[tokenId] = Weather(weather);
​
emit NftWeatherUpdated(tokenId, Weather(weather));
​
+ // Clean up state for the completed update request
+ delete s_funcReqIdToTokenIdUpdate[requestId];
}
​
// Note: Consider the case in fulfillRequest if a requestId is neither for mint nor update.
// Currently, it does nothing. If s_funcReqIdToUserMintReq[requestId].user was address(0)
// AND s_funcReqIdToTokenIdUpdate[requestId] was 0, then fulfillRequest completes without action.
// This is fine, but ensuring all paths clean up their specific request IDs is key.
// The above changes target the primary usage paths.
Updates

Appeal created

bube Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Multiple tokens for one `requestId`

The `WeatherNFT::fulfillMintRequest` allows a malicious user to call multiple times the function with the same `requestId`.

Support

FAQs

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