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

Missing withdrawal functionality in WeatherNFT results in permanent lockup of minting proceeds.

blackgrease

Summary

The WeatherNFT contract accepts funds via the payable function requestMintWeatherNFT. However, there is no withdraw functionality for accumulated funds resulting in them being locked permanently in the contract.

Description

To mint a Weather NFT, a user is required to call the payable function, WeatherNft:requestMintWeatherNFT with an amount equal to the current minting price.
Overtime, with more users minting, the WeatherNFT contract will accumulate funds.

However, the contract has no function to withdraw the funds in the contract resulting in funds being permanently locked in the contract and can not be used by the owner/protocol for any future ventures and/or profit.

Review of existing external/public functions

Below is a list of all external and public functions in the contract

  1. requestMintWeatherNFT

  2. fulfillMintRequest

  3. checkUpkeep

  4. performUpkeep

  5. updateFunctionsGasLimit

  6. updateSubId

  7. updateMinimumLinkDeposit

  8. updateSource

  9. updateEncryptedSecretsURL

  10. updateKeeperGaslimit

  11. tokenURI

None of the above contracts have any logic related to withdrawing the funds in the contract.

Any internal functions linked with above mentioned external/public functions, also have no logic related to withdrawals.

Risk / Impact

This is High Severity issue due to:

High Impact: There are lost funds for the protocol

  • The funds within the contract can not be used to collect profit for the protocol. Furthermore, the locked funds cannot be used to fund any future developments to improve the protocol.

High Likelihood: This issue will occur every time a user mints request to mint an NFT.

  • There are no complications or additional requirements meaning each time, a user pays to have an NFT minted, the contract will have the balance of the locked funds incremented.

Proof of Concept

To prove the validity of the issue a manual walkthrough is provided.

  1. Minting price is at 0.5 ether. User A calls requestMintWeatherNFT with 0.5 ether to pay for minting.

  2. 3 more users pay to have a WeatherNFT. (each paying slightly more than the previous user)

  3. The Protocol/Owner tries to withdraw the funds but cannot because there is no withdraw functionality.

Code

While the issue may suffice from a manual walkthrough, I have provided a test case than is compatible with the current test suite. It simply shows the contract balance exists when user mints an NFT. These funds will be permanently locked in WeatherNFT.

​
function testLockedFundsInContract() public {
uint256 currentMintPrice = weatherNft.s_currentMintPrice();
​
uint256 startingContractBalance = address(weatherNft).balance;
​
/////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///////
//Any other functions called at this point are irrelevant
uint256 finalContractBalance = address(weatherNft).balance;
​
/////Assert////
assert(finalContractBalance > startingContractBalance);
​
//Logging
console.log("Starting contract Balance: ", startingContractBalance);
console.log("Final contract Balance: ", finalContractBalance);
}

Recommended Mitigation

The recommended mitigation to solve this issue is to add a withdraw method that can only be called by the owner.

I have provided a function template that can be added to the contract.

  1. It can only be called by the Owner

  2. It requires the contract balance to be greater than 0

​
function withdrawFunds() external onlyOwner{
uint256 contractBalance = address(this).balance;
require(contractBalance > 0, "Not enough Funds to withdraw");
​
address contractOwner = owner();
​
(bool success,) = payable(contractOwner).call{value: contractBalance}("");
require(success, "Failed to withdraw funds");
}
​
Updates

Appeal created

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

Lack of `withdraw` function

The contract collects funds for minting a WeatherNFT, but there is no function that allows the owner to withdraw these funds.

Support

FAQs

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