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

`fulfillMintRequest` is public and callable by anyone

k0ns0l

Description

  • The function is marked external and has no access control. Anyone who knows the requestIdcan call it, not just the original requester.

  • Ultimately the NFT is minted to msg.sender, which MAY not be the original requester.

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) {
return;
}
​
@> UserMintRequest memory _userMintRequest = s_funcReqIdToUserMintReq[
@> requestId
];
uint8 weather = abi.decode(response, (uint8));
uint256 tokenId = s_tokenCounter;
s_tokenCounter++;
​
emit WeatherNFTMinted(requestId, msg.sender, Weather(weather));
@> _mint(msg.sender, tokenId);
s_tokenIdToWeather[tokenId] = Weather(weather);
​
uint256 upkeepId;
if (_userMintRequest.registerKeeper) {
// Register chainlink keeper to pull weather data in order to automate weather nft
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)
});
​
upkeepId = IAutomationRegistrarInterface(s_keeperRegistrar)
.registerUpkeep(_keeperParams);
}

Risk

Likelihood:

  • It trusts that the var response, or error has been properly set (by a Chainlink Functions callback) for each call - requestMintWeatherNFT

  • But the contract does not restrict who can call fulfillMintRequest.

Impact:

  • Frontrunning, when someone else can mint the NFT before the intended user

  • repeatedly calling thefulfillMintRequestfor valid requestId can result to DoS

Proof of Concept

function test_anyoneCanClaimMintedNFT() public {
// Let user initiate a mint request
uint256 mintPrice = weatherNft.s_currentMintPrice();
vm.prank(user);
bytes32 reqId = weatherNft.requestMintWeatherNFT{value: mintPrice}(
"12345",
"US",
false,
0,
0
);
​
bytes memory response = abi.encode(uint8(1)); // e.g., weather = 1
bytes memory err = "";
//
weatherNft.fulfillRequest(reqId, response, err);
​
// front-runs and calls fulfillMintRequest with the user's requestId
vm.prank(attacker);
weatherNft.fulfillMintRequest(reqId);
​
// Assert that attacker owns the NFT, not the original user
uint256 tokenId = weatherNft.s_tokenCounter() - 1;
assertEq(weatherNft.ownerOf(tokenId), attacker);
}

Recommended Mitigation

  • Move fulfillMintRequest into fulfillRequest, and make it internal. This should ensure only Chainlink's fulfillRequest can trigger the minting.

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, err: err});
+ _fulfillMintRequest(requestId);
} else if (s_funcReqIdToTokenIdUpdate[requestId] > 0)
+ // <SNIP>
}
​
​
​
+ /// @notice refactored fulfillMintRequest -
+ function _fulfillMintRequest(bytes32 requestId) internal {
+ // <SNIP>
+ // Additional check to make sure of caller
- emit WeatherNFTMinted(requestId, msg.sender, Weather(weather));
- _mint(msg.sender, tokenId);
+ emit WeatherNFTMinted(requestId, msg.sender, Weather(weather));
+ _mint(_userMintRequest.user, tokenId);
+ // <SNIP>
+ }
Updates

Appeal created

bube Lead Judge 4 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Lack of ownership check in `fulfillMintRequest` function

There is no check to ensure that the caller of the `fulfillMintRequest` function is actually the owner of the `requestId`. This allows a malicious user to receive a NFT that is payed from someone else.

Support

FAQs

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