_mint() to smart contract might cause loss of nft
Description
When a user calls fulfillMintRequest(), the _mint() function is used to transfer the newly created NFT to msg.sender. While External Owned Accounts (EOAs) can inherently receive and manage ERC721 tokens, smart contract addresses require specific logic to safely handle incoming tokens. If msg.sender is a smart contract that does not implement the IERC721Receiver interface (specifically the onERC721Received() callback function) or does not handle it correctly, the minted token will be transferred to that contract's address but become permanently inaccessible, as the contract has no mechanism to acknowledge or manage it.
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];
require(_userMintRequest.user == msg.sender, WeatherNft__Unauthorized());
uint8 weather = abi.decode(response, (uint8));
uint256 tokenId = s_tokenCounter;
s_tokenCounter++;
emit WeatherNFTMinted(requestId, msg.sender, Weather(weather));
s_tokenIdToWeather[tokenId] = Weather(weather);
}
Risk
Likelihood: medium
-
Users might interact with the contract through non-compliant smart contract wallets, multi-signature wallets without onERC721Received implemented, or custom proxy contracts that are not designed to receive ERC721 tokens safely.
-
Reason 2
Impact: high
-
Permanent loss of the minted NFT. The token will exist on-chain and be owned by the recipient contract address, but the contract will be unable to transfer, sell, or otherwise manage the token, effectively locking it forever.
-
Financial loss and poor user experience for the legitimate user. The user would have paid the required mint price (and potentially other fees) but would not receive a usable or accessible NFT, leading to frustration and distrust in the protocol.
Proof of Concept
contract NonCompliantNFTReceiver {
address public weatherNftAddress;
constructor(address _weatherNftAddress) {
weatherNftAddress = _weatherNftAddress;
}
function callRequestMint(
string memory _pincode,
string memory _isoCode,
bool _registerKeeper,
uint256 _heartbeat,
uint256 _initLinkDeposit,
uint256 _mintPrice
) external payable {
WeatherNft(weatherNftAddress).requestMintWeatherNFT{value: _mintPrice}(
_pincode, _isoCode, _registerKeeper, _heartbeat, _initLinkDeposit
);
}
function callFulfillMint(bytes32 requestId) external {
WeatherNft(weatherNftAddress).fulfillMintRequest(requestId);
}
}
function test_nonCompliantContractCannotHandleNFT() public {
vm.startPrank(user);
NonCompliantNFTReceiver nonCompliantReceiver = new NonCompliantNFTReceiver(address(weatherNft));
vm.stopPrank();
vm.deal(address(nonCompliantReceiver), 100e18);
console.log("NonCompliantReceiver deployed at:", address(nonCompliantReceiver));
console.log("NonCompliantReceiver balance:", address(nonCompliantReceiver).balance);
string memory pincode = "90210";
string memory isoCode = "US";
bool registerKeeper = false;
uint256 heartbeat = 1 hours;
uint256 mintPrice = weatherNft.s_currentMintPrice();
uint256 expectedTokenId = weatherNft.s_tokenCounter();
console.log("Expected Token ID to be minted:", expectedTokenId);
vm.startPrank(address(nonCompliantReceiver));
vm.recordLogs();
nonCompliantReceiver.callRequestMint{value: mintPrice}(
pincode, isoCode, registerKeeper, heartbeat, 0, mintPrice
);
vm.stopPrank();
Vm.Log[] memory logs = vm.getRecordedLogs();
bytes32 mintReqId;
for (uint256 i; i < logs.length; i++) {
if (logs[i].topics[0] == keccak256("WeatherNFTMintRequestSent(address,string,string,bytes32)")) {
(,,, mintReqId) = abi.decode(logs[i].data, (address, string, string, bytes32));
break;
}
}
assert(mintReqId != bytes32(0));
vm.prank(functionsRouter);
bytes memory weatherResponse = abi.encode(WeatherNftStore.Weather.RAINY);
weatherNft.handleOracleFulfillment(mintReqId, weatherResponse, "");
console.log("Chainlink Functions fulfillment simulated.");
vm.startPrank(address(nonCompliantReceiver));
nonCompliantReceiver.callFulfillMint(mintReqId);
vm.stopPrank();
console.log("NonCompliantReceiver called fulfillMintRequest.");
assertEq(
weatherNft.ownerOf(expectedTokenId),
address(nonCompliantReceiver),
"NFT should be minted to the non-compliant contract address."
);
console.log("NFT (ID:", expectedTokenId, ") owned by:", weatherNft.ownerOf(expectedTokenId));
}
Recommended Mitigation
Replace the _mint() function call with _safeMint() in fulfillMintRequest(). The _safeMint() function, provided by OpenZeppelin's ERC721 implementation, includes a check that ensures the recipient address can safely receive ERC721 tokens (by calling onERC721Received() if the recipient is a contract). If the recipient contract does not implement this function or returns an invalid value, the transaction will revert, preventing the token from being irretrievably locked.
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];
require(_userMintRequest.user == msg.sender, WeatherNft__Unauthorized());
uint8 weather = abi.decode(response, (uint8));
uint256 tokenId = s_tokenCounter;
s_tokenCounter++;
emit WeatherNFTMinted(requestId, msg.sender, Weather(weather));
- _mint(msg.sender, tokenId);
+ _safeMint(msg.sender, tokenId);
s_tokenIdToWeather[tokenId] = Weather(weather);
// .. rest of logic
}
