Weather Witness
First Flight #40
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

[H-3] The ```WeatherNft:::checkUpkeep``` Function Has Missing Access Control.

wolf_kalp

Root + Impact

The WeatherNft:::checkUpkeep Function Has Missing Access Control.

Description

The WeatherNft:::checkUpkeep Function Has Missing Access Control.The WeatherNft:::checkUpkeep should
Only Be Called By The Chainlink Nodes and owner of Token(NFT) .Anyone Can Call checkUpkeep Function
And can Update The States.The Documentation Says That checkUpkeep Function Should Only be Called By
ChainLink Keepers For Those Users Who Had registered For Automation.

Impact:

Due To Missing Access Control In WeatherNft:::checkUpkeep Function.Any Malicious User/Attacker Can Call the Function repeatedly and can update The malicious States and leads to unnecessary gas usage.

Proof of Concept

function testAnyOneCanCallcheckupkeep() 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);
​
bytes32 reqid = weatherNft.requestMintWeatherNFT{
value: weatherNft.s_currentMintPrice()
}(pincode, isoCode, registerKeeper, heartbeat, initLinkDeposit);
vm.stopPrank();
​
bytes memory weatherResponse = abi.encode(
WeatherNftStore.Weather.RAINY
);
weatherNft.handleOracleFulfillment(reqid, weatherResponse, "");
​
vm.prank(user);
weatherNft.fulfillMintRequest(reqid);
​
vm.prank(user);
string memory tokenURI = weatherNft.tokenURI(tokenId);
​
bytes memory encodedTokenId = abi.encode(tokenId);
vm.warp(block.timestamp + 12 hours);
vm.prank(malicioussuser);
(bool weatherUpdateNeeded, bytes memory performdata) = weatherNft
.checkUpkeep(encodedTokenId);
/* Ran 1 test for test/WeatherNftForkTest.t.sol:WeatherNftForkTest
[PASS] testAnyOneCanCallcheckupkeep() (gas: 869190)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.16ms (1.66ms CPU time)
Ran 1 test suite in 22.08ms (5.16ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
*/
}

Recommended Mitigation

+ +require(s_keeperRegistry == msg.sender || ownerOf(tokenId) == msg.sender,"NotAuthorized");
function checkUpkeep(
bytes calldata checkData
)
external
view
override
returns (bool upkeepNeeded, bytes memory performData)
{
uint256 _tokenId = abi.decode(checkData, (uint256));
​
require(
s_keeperRegistry == msg.sender || ownerOf(_tokenId) == msg.sender,
"NotAuthorized"
);
if (_ownerOf(_tokenId) == address(0)) {
upkeepNeeded = false;
} else {
upkeepNeeded = (block.timestamp >=
s_weatherNftInfo[_tokenId].lastFulfilledAt +
s_weatherNftInfo[_tokenId].heartbeat);
if (upkeepNeeded) {
performData = checkData;
}
}
}
Updates

Appeal created

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

[Invalid] Lack of access control in `checkUpkeep` function

After discussion with the sponsor, I have downgraded this to "Informational/Invalid". The `checkUpkeep` function is a view function and it returns if a `tokenId` needs a weather update, it doesn't modify the state and there is no impact of calling this function. The problem is the missing access control in `performUpkeep` function.

bube Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[Invalid] Lack of access control in `checkUpkeep` function

After discussion with the sponsor, I have downgraded this to "Informational/Invalid". The `checkUpkeep` function is a view function and it returns if a `tokenId` needs a weather update, it doesn't modify the state and there is no impact of calling this function. The problem is the missing access control in `performUpkeep` function.

Support

FAQs

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