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

Unfair Mint Price Increase on Failed Mint Attempts

albert

Root + Impact

Description

The requestMintWeatherNFT function increments s_currentMintPrice by s_stepIncreasePerMint immediately upon being called, before the Chainlink Functions request to fetch weather data is initiated and long before the NFT is actually minted in fulfillMintRequest.
If the Chainlink Functions call fails (e.g., oracle error, JavaScript error in GetWeather.js, API unavailability) or if fulfillMintRequest subsequently fails for any reason (e.g., invalid data from oracle, keeper registration failure), the minting process is aborted. However, s_currentMintPrice is not decremented back to its original value. This means that the user who experienced the failed mint, or any subsequent user, will face a higher mint price for their next attempt, even though the previous attempt did not result in a successful NFT mint. This penalizes users for system or oracle failures beyond their control.

// 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 current price
WeatherNft__InvalidAmountSent()
);
// @> s_currentMintPrice += s_stepIncreasePerMint; // Price is increased here, *before* oracle call or minting success
​
if (_registerKeeper) {
IERC20(s_link).safeTransferFrom(
msg.sender,
address(this),
_initLinkDeposit
);
}
​
_reqId = _sendFunctionsWeatherFetchRequest(_pincode, _isoCode); // Oracle request, can fail
​
emit WeatherNFTMintRequestSent(msg.sender, _pincode, _isoCode, _reqId);
​
s_funcReqIdToUserMintReq[_reqId] = UserMintRequest({
user: msg.sender,
pincode: _pincode,
isoCode: _isoCode,
registerKeeper: _registerKeeper,
heartbeat: _heartbeat,
initLinkDeposit: _initLinkDeposit
// This stored request might never lead to a successful mint.
});
}
​
// In fulfillMintRequest:
// If an error occurs (oracle error, invalid response, keeper registration fails),
// the function may return early or revert. s_currentMintPrice is NOT adjusted back.
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 or returned an error
// ... funds are potentially lost (separate issue)
// ... s_currentMintPrice remains at the incremented value
return;
}
// ... further processing, which could also revert (e.g. bad enum from oracle, keeper registration fail)
// If any revert happens here, s_currentMintPrice remains incremented.
}

Risk

Likelihood: High

  • Oracle calls or subsequent minting steps can fail due to various reasons (API issues, JavaScript errors, Chainlink node issues, misconfigurations, gas problems). Each such failure will leave the mint price unfairly inflated.

Impact: Medium

  • Financial Penalty for Users: Users trying to mint again after a failure, or new users, have to pay a higher price that was increased due to a system error, not a successful mint.

  • Deteriorating User Experience: Makes the minting process appear unreliable and potentially costly if multiple attempts are needed due to external factors.

  • Potential for Price Griefing (Minor): A malicious actor could repeatedly initiate mint requests they know might fail (e.g., by providing parameters likely to cause an error in GetWeather.js if such inputs exist, or by trying to front-run/disrupt the oracle response) to drive up the price for legitimate users, although this is less direct. The primary issue is the unfair increase from non-malicious failures.

Proof of Concept

  1. s_currentMintPrice is 1 ETH. s_stepIncreasePerMint is 0.1 ETH.

  2. Alice calls requestMintWeatherNFT sending 1 ETH. Inside the function, s_currentMintPrice becomes 1.1 ETH.

  3. The Chainlink Functions call made on Alice's behalf fails (e.g., GetWeather.js throws an error, or OpenWeather API is down).

  4. The oracle returns an error to fulfillRequest, which populates s_funcReqIdToMintFunctionReqResponse[requestId].err.

  5. Alice (or an automated system) calls fulfillMintRequest(requestId). The function sees err.length > 0 and returns. Alice does not get an NFT. Her 1 ETH might be stuck (as per another reported issue).

  6. Bob now wants to mint. He checks s_currentMintPrice and sees it is 1.1 ETH. He has to pay 0.1 ETH more because Alice's mint attempt failed due to an oracle error.

Recommended Mitigation

Increment s_currentMintPrice only after a successful mint has occurred (i.e., at the end of fulfillMintRequest just before NFT is minted or state is finalized for the new NFT).

// 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,
WeatherNft__InvalidAmountSent()
);
- // s_currentMintPrice += s_stepIncreasePerMint; // Moved to fulfillMintRequest
​
if (_registerKeeper) {
IERC20(s_link).safeTransferFrom(
msg.sender,
address(this),
_initLinkDeposit
);
}
​
_reqId = _sendFunctionsWeatherFetchRequest(_pincode, _isoCode);
​
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) {
// TODO: Handle refund for msg.value from requestMintWeatherNFT if applicable.
// Clean up state
delete s_funcReqIdToUserMintReq[requestId];
delete s_funcReqIdToMintFunctionReqResponse[requestId];
return;
}
​
UserMintRequest memory _userMintRequest = s_funcReqIdToUserMintReq[requestId];
require(_userMintRequest.user != address(0), "WeatherNft__InvalidRequest"); // Assuming fix for user attribution
​
uint8 weather_uint = abi.decode(response, (uint8));
// Add check for valid enum range if GetWeather.js cannot guarantee it.
// require(weather_uint < 6, "WeatherNft__InvalidWeatherEnumValue"); // Example: 6 is number of Weather states
Weather weather_enum = Weather(weather_uint);
uint256 tokenId = s_tokenCounter;
​
// Mint the NFT
_mint(_userMintRequest.user, tokenId); // Mint to the original requestor
s_tokenIdToWeather[tokenId] = weather_enum;
​
// Increment price and counter *after* successful setup leading to mint
+ s_currentMintPrice += s_stepIncreasePerMint;
+ s_tokenCounter++;
​
​
uint256 upkeepId;
if (_userMintRequest.registerKeeper) {
LinkTokenInterface(s_link).approve(s_keeperRegistrar, _userMintRequest.initLinkDeposit);
​
IAutomationRegistrarInterface.RegistrationParams
memory _keeperParams = IAutomationRegistrarInterface
.RegistrationParams({
name: string.concat(
"Weather NFT Keeper: ",
Strings.toString(tokenId)
),
encryptedEmail: "",
upkeepContract: address(this),
gasLimit: s_upkeepGaslimit,
adminAddress: address(this),
triggerType: 0,
checkData: abi.encode(tokenId),
triggerConfig: "",
offchainConfig: "",
amount: uint96(_userMintRequest.initLinkDeposit) // Potential truncation issue here (separate vulnerability)
});
​
upkeepId = IAutomationRegistrarInterface(s_keeperRegistrar)
.registerUpkeep(_keeperParams);
}
​
s_weatherNftInfo[tokenId] = WeatherNftInfo({
heartbeat: _userMintRequest.heartbeat,
lastFulfilledAt: block.timestamp,
upkeepId: upkeepId,
pincode: _userMintRequest.pincode,
isoCode: _userMintRequest.isoCode
});
emit WeatherNFTMinted(
requestId,
_userMintRequest.user,
weather_enum
);
​
// Clean up state for the completed mint request
delete s_funcReqIdToUserMintReq[requestId];
delete s_funcReqIdToMintFunctionReqResponse[requestId];
}
​

Self-correction during mitigation suggestion: s_tokenCounter should also be incremented only upon successful mint, not when fulfillMintRequest is entered. I've adjusted the diff for s_tokenCounter++ to be near s_currentMintPrice += s_stepIncreasePerMint;.

Updates

Appeal created

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

The price of the token is increased before the token is minted

Support

FAQs

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