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

User Loses Mint Fee and LINK Deposit if Oracle Call Fails or Minting Process Reverts

albert

Root + Impact

Description

When a user calls requestMintWeatherNFT, they pay s_currentMintPrice and, if _registerKeeper is true, transfer _initLinkDeposit amount of LINK tokens to the contract. The contract then initiates a Chainlink Functions request. If this oracle request fails (e.g., JavaScript error, API issue, invalid API key, out-of-range enum returned by JS) or if any subsequent step in fulfillMintRequest reverts (e.g., keeper registration fails, or Weather(weather) conversion fails due to an out-of-range enum value from the oracle response), the fulfillMintRequest function will either return early or revert. In such cases, the NFT is not minted, but the mint fee (msg.value) and the deposited LINK tokens are not refunded to the user, resulting in a loss of funds.

// Root cause in the codebase with @> marks to highlight the relevant section
// File: src/WeatherNft.sol
​
function requestMintWeatherNFT(
string memory _pincode,
string memory _isoCode,
bool _registerKeeper,
uint256 _heartbeat,
uint256 _initLinkDeposit
) external payable returns (bytes32 _reqId) {
require(
// @> msg.value == s_currentMintPrice, // User pays mint price
WeatherNft__InvalidAmountSent()
);
s_currentMintPrice += s_stepIncreasePerMint;
​
if (_registerKeeper) {
IERC20(s_link).safeTransferFrom(
msg.sender,
address(this),
// @> _initLinkDeposit // User deposits LINK to the contract
);
}
​
_reqId = _sendFunctionsWeatherFetchRequest(_pincode, _isoCode); // Oracle request initiated
​
emit WeatherNFTMintRequestSent(msg.sender, _pincode, _isoCode, _reqId);
​
s_funcReqIdToUserMintReq[_reqId] = UserMintRequest({
user: msg.sender,
pincode: _pincode,
isoCode: _isoCode,
registerKeeper: _registerKeeper,
heartbeat: _heartbeat,
initLinkDeposit: _initLinkDeposit
});
}
​
function fulfillMintRequest(bytes32 requestId) external {
bytes memory response = s_funcReqIdToMintFunctionReqResponse[requestId].response;
bytes memory err = s_funcReqIdToMintFunctionReqResponse[requestId].err;
​
require(response.length > 0 || err.length > 0, WeatherNft__Unauthorized());
​
// @> if (response.length == 0 || err.length > 0) { // Oracle call failed (e.g. JS error)
// @> return; // Exits without minting NFT or refunding fees/LINK
}
​
UserMintRequest memory _userMintRequest = s_funcReqIdToUserMintReq[requestId];
uint8 weather_uint = abi.decode(response, (uint8));
// If weather_uint is out of range for Weather enum (e.g., > 5), Weather(weather_uint) will revert.
// This causes fulfillMintRequest to revert.
// @> Weather weather_enum = Weather(weather_uint); // Potential revert point
​
uint256 tokenId = s_tokenCounter;
s_tokenCounter++; // Incremented even if subsequent operations revert
​
_mint(_userMintRequest.user, tokenId); // Assuming previous fix for user
s_tokenIdToWeather[tokenId] = weather_enum;
​
uint256 upkeepId;
if (_userMintRequest.registerKeeper) {
LinkTokenInterface(s_link).approve(s_keeperRegistrar, _userMintRequest.initLinkDeposit);
// ...
// @> upkeepId = IAutomationRegistrarInterface(s_keeperRegistrar).registerUpkeep(_keeperParams); // Potential revert point if keeper registration fails
}
// If any step above reverts, the user's ETH (mint fee) and LINK (if deposited) remain in the contract.
// ...
}
​
function fulfillRequest(
bytes32 requestId,
bytes memory response,
bytes memory err
) internal override {
if (s_funcReqIdToUserMintReq[requestId].user != address(0)) {
s_funcReqIdToMintFunctionReqResponse[requestId] = MintFunctionReqResponse({
response: response, // This might contain an out-of-range enum value
err: err // This might contain an error from the oracle
});
}
// ...
}
​
// GetWeather.js (Conceptual based on README)
// If GetWeather.js returns a weather_enum value (e.g., 6, 7) that is not defined in `WeatherNftStore.Weather`,
// the `Weather(weather)` conversion in `fulfillMintRequest` will revert.
// The README notes: "The calculation of weather_enum in GetWeather.js is limited and set to windy in else statement"
// This implies some weather conditions might not be handled gracefully, potentially leading to errors or unexpected enum values.

Risk

Likelihood: Medium

  • Oracle (Chainlink Functions) errors can occur due to issues with the JavaScript source, external API unavailability/errors (OpenWeather API), invalid API keys, or insufficient LINK in the Functions subscription.

  • The GetWeather.js script might return an enum value outside the valid range [0-5], causing Weather(weather_uint) to revert.

  • Keeper registration can fail due to misconfiguration or registrar issues.

Impact: High

  • Loss of User Funds: Users lose their mint fee (ETH) and any LINK tokens deposited for automation services without receiving an NFT.

  • Poor User Experience: Users are penalized for failures in external dependencies or contract interactions that are beyond their direct control during the minting phase.

Proof of Concept

  1. Alice calls requestMintWeatherNFT, paying 0.1 ETH (example s_currentMintPrice) and depositing 5 LINK (_initLinkDeposit). These funds are now held by the WeatherNft contract.

  2. The Chainlink Function executes GetWeather.js.
    Scenario A: GetWeather.js encounters an error (e.g., OpenWeather API is down) and returns an error (err in fulfillRequest).
    Scenario B: GetWeather.js successfully fetches data but incorrectly calculates weather_enum to be 7 (an invalid enum value).

  3. The Oracle calls handleOracleFulfillment -> fulfillRequest.
    In Scenario A: s_funcReqIdToMintFunctionReqResponse[reqId].err is populated.
    In Scenario B: s_funcReqIdToMintFunctionReqResponse[reqId].response is populated with abi.encode(7).

  4. Alice (or a service) calls fulfillMintRequest(reqId).
    In Scenario A: The function executes if (response.length == 0 || err.length > 0) { return; }. Alice's 0.1 ETH and 5 LINK remain in the contract. No NFT is minted.
    In Scenario B: uint8 weather_uint = abi.decode(response, (uint8)) decodes 7. The call Weather(weather_uint) (i.e., Weather(7)) reverts because 7 is not a valid member of the Weather enum. The entire fulfillMintRequest transaction reverts. Alice's 0.1 ETH and 5 LINK remain in the contract. No NFT is minted. s_tokenCounter might have been incremented before the revert, skipping a token ID.

Recommended Mitigation

Implement mechanisms to refund the user's mint fee and LINK deposit if the minting process fails after payment. This could involve:

  1. For Oracle Errors (err.length > 0): Allow the user to claim a refund of their mint fee and LINK deposit.

  2. For Reverts within fulfillMintRequest (e.g., invalid enum, keeper registration failure): Since the transaction reverts, funds paid as msg.value are automatically returned to the caller of fulfillMintRequest. However, LINK tokens previously transferred to the contract via requestMintWeatherNFT would remain. A separate claim/refund mechanism would be needed for this LINK.

  3. Robust Off-Chain Logic: Ensure GetWeather.js strictly returns valid enum values (0-5) and handles external API errors gracefully, perhaps by returning a designated "UNKNOWN" or default weather state instead of an out-of-range value or raw error that causes contract reverts.

A more comprehensive solution would involve a try-catch pattern for critical operations within fulfillMintRequest or a two-phase commit where funds are only truly committed upon successful minting. Given Solidity's limitations, a refund mechanism is more practical.

Example for handling Oracle errors:

// File: src/WeatherNft.sol
// Add a mapping to track refundable amounts
mapping(bytes32 => uint256) public s_refundableMintFee;
mapping(bytes32 => uint256) public s_refundableLinkDeposit;
​
function requestMintWeatherNFT(
string memory _pincode,
string memory _isoCode,
bool _registerKeeper,
uint256 _heartbeat,
uint256 _initLinkDeposit
) external payable returns (bytes32 _reqId) {
require(
msg.value == s_currentMintPrice,
WeatherNft__InvalidAmountSent()
);
// Temporarily store potential refund amounts
// Note: This is a simplified example; managing request lifecycle for refunds needs care
// s_refundableMintFee[_reqId] = msg.value; // This will be complex with current price logic
// if (_registerKeeper) {
// s_refundableLinkDeposit[_reqId] = _initLinkDeposit;
// }
​
​
// ... (rest of the function, including LINK transfer)
// Actual LINK transfer should ideally happen in fulfillMintRequest, or a robust refund path for LINK is needed.
// For now, let's focus on the return path in fulfillMintRequest.
}
​
​
function fulfillMintRequest(bytes32 requestId) external {
// ... (assuming user fix from previous vulnerability)
UserMintRequest memory _userMintRequest = s_funcReqIdToUserMintReq[requestId];
require(_userMintRequest.user != address(0), "WeatherNft__InvalidRequest");
​
bytes memory response = s_funcReqIdToMintFunctionReqResponse[requestId].response;
bytes memory err = s_funcReqIdToMintFunctionReqResponse[requestId].err;
​
require(response.length > 0 || err.length > 0, WeatherNft__Unauthorized());
​
if (response.length == 0 || err.length > 0) {
// Oracle error occurred.
// Option 1: Allow user to claim a refund (requires more state and a refund function)
// emit MintFailedOracleError(requestId, _userMintRequest.user, _userMintRequest.initLinkDeposit);
// The ETH for mint fee is already in the contract.
// The LINK for deposit is already in the contract.
// A separate function `claimRefund(requestId)` would be needed.
​
// Option 2: Simpler, but less flexible - attempt to refund directly if possible (complex with gas)
// For now, this part shows where the problem path is:
// The user's funds are stuck if we just return.
// Critical: Clean up state to prevent replay or misuse
delete s_funcReqIdToUserMintReq[requestId];
delete s_funcReqIdToMintFunctionReqResponse[requestId];
// TODO: Implement refund logic for _userMintRequest.user for their initial mint fee and LINK deposit.
// This would likely involve transferring ETH back to _userMintRequest.user
// and LINK (if _userMintRequest.registerKeeper) back to _userMintRequest.user.
// For example:
// payable(_userMintRequest.user).transfer(s_currentMintPrice); // Price at time of request would be needed
// if (_userMintRequest.registerKeeper && _userMintRequest.initLinkDeposit > 0) {
// LinkTokenInterface(s_link).transfer(_userMintRequest.user, _userMintRequest.initLinkDeposit);
// }
return;
}
​
// ... rest of the minting logic ...
// If Weather(weather_uint) or registerUpkeep reverts, the transaction reverts.
// msg.value of fulfillMintRequest is returned to its caller.
// However, the _initLinkDeposit (if any) and the original msg.value for requestMintWeatherNFT
// are still in the contract. This requires a refund path.
​
// Clean up state
delete s_funcReqIdToUserMintReq[requestId];
delete s_funcReqIdToMintFunctionReqResponse[requestId];
}
​
// A new function would be needed for users to claim refunds if fulfillMintRequest can't refund directly.
// function claimFailedMintRefund(bytes32 requestId) external {
// // 1. Verify that 'requestId' indeed failed and is eligible for refund.
// // 2. Verify msg.sender is the original user for that requestId.
// // 3. Transfer the stored mint fee (ETH) and LINK deposit back to the user.
// // 4. Clean up refund eligibility state.
// }

A robust refund mechanism is non-trivial and needs careful design to prevent exploits (e.g., double withdrawal, refunding successful mints). A simpler immediate fix is to ensure GetWeather.js cannot cause reverts by returning valid enums or a default error enum. For LINK deposits, if registerUpkeep fails, that LINK is stuck without a withdrawal function.

Updates

Appeal created

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

Lost fee in case of Oracle failure

If Oracle fails, the `fulfillMintRequest` function will not return the payed fee for the token to the user.

Support

FAQs

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