[M-3] Owner Can Update Source Code Without Owner Can Update Source Code Without Constrains, Causing Potential Data Manipulation and NFT Value Exploitation
Description
The WeatherNft contract relies on Chainlink Functions to execute external JavaScript code (stored in s_functionsConfig.source) that fetches weather data from the OpenWeather API and translates weather codes into the NFT's weather states. This source code is critical for ensuring accurate and trustworthy weather data representation in the NFTs.
The contract allows the owner to update this JavaScript source code instantly without any timelock, verification period, or multi-signature requirement, creating a single point of failure where a compromised owner account could immediately deploy malicious code that returns false weather data.
function updateSource(string memory newSource) external onlyOwner {
@> s_functionsConfig.source = newSource;
}
Risk
Likelihood: Medium
Owner private keys are high-value targets and can be compromised through phishing, social engineering, or insider threats.
Admin key management systems sometimes fail due to operational security lapses or technical errors.
During contract management operations, owners frequently update source code to fix bugs or add features.
Impact: High
Malicious source code could return fake weather data, undermining the core value proposition of the NFT collection that represents "real-world weather conditions."
The JavaScript source (GetWeather.js) contains critical mapping logic that converts OpenWeatherMap codes to NFT weather states - manipulation of this logic could artificially skew weather distributions.
Weather data could be manipulated to favor specific NFT holders, creating an unfair advantage in any weather-dependent applications or games built on top of the NFTs.
User trust in the entire ecosystem would be severely damaged when discovering the centralized point of failure.
NFT values could be artificially inflated or deflated by returning specific weather conditions that make certain NFTs more desirable.
Proof of Concept
Add the following test to the testing suite, and it will prove that, no matter what the owner adds, it will still go through, updating the source code with something, that could potentially steal private keys or funds.
function test_owner_can_manipulate_weather_with_malicious_source() public {
string memory pincode = "125001";
string memory isoCode = "IN";
bool registerKeeper = false;
uint256 heartbeat = 12 hours;
uint256 initLinkDeposit = 0;
uint256 tokenId = weatherNft.s_tokenCounter();
vm.startPrank(user);
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;
}
}
vm.prank(functionsRouter);
bytes memory legit_weather = abi.encode(WeatherNftStore.Weather.SUNNY);
weatherNft.handleOracleFulfillment(reqId, legit_weather, "");
vm.prank(user);
weatherNft.fulfillMintRequest(reqId);
assertEq(
uint8(weatherNft.s_tokenIdToWeather(tokenId)),
uint8(WeatherNftStore.Weather.SUNNY)
);
string memory malicious_js = "bad code";
vm.prank(owner);
weatherNft.updateSource(malicious_js);
vm.warp(block.timestamp + heartbeat);
vm.recordLogs();
weatherNft.performUpkeep(abi.encode(tokenId));
logs = vm.getRecordedLogs();
bytes32 updateReqId;
for (uint256 i; i < logs.length; i++) {
if (
logs[i].topics[0] ==
keccak256(
"NftWeatherUpdateRequestSend(uint256,bytes32,uint256)"
)
) {
(, updateReqId) = abi.decode(logs[i].data, (uint256, bytes32));
break;
}
}
vm.prank(functionsRouter);
bytes memory manipulated_weather = abi.encode(
WeatherNftStore.Weather.SNOW
);
weatherNft.handleOracleFulfillment(
updateReqId,
manipulated_weather,
""
);
assertEq(
uint8(weatherNft.s_tokenIdToWeather(tokenId)),
uint8(WeatherNftStore.Weather.SNOW)
);
console.log("Original weather: SUNNY (0)");
console.log("Weather after malicious source update: SNOW (5)");
console.log(
"The owner successfully manipulated weather data without delay or oversight"
);
}
Recommended Mitigation
As mentioned in another finding that talks about the centralization issue, implementing a timelock mechanism or off-chain voting could technically solve this issue, but ideally, centralization should be avoided in most cases.
+ import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";
+ TimelockController public immutable timelock;
+ string public pendingSource;
+ uint256 public constant SOURCE_UPDATE_DELAY = 2 days;
constructor(
// ...
+ address[] memory proposers,
+ address[] memory executors
) ERC721("Weather NFT", "W-NFT") FunctionsClient(functionsRouter) ConfirmedOwner(msg.sender) {
// ...
+ timelock = new TimelockController(
+ SOURCE_UPDATE_DELAY,
+ proposers,
+ executors,
+ address(0) // No admin delay
+ );
}
- function updateSource(string memory newSource) external onlyOwner {
- s_functionsConfig.source = newSource;
- }
+ /**
+ * @notice Proposes a new source code update, which must go through timelock
+ * @dev Only callable by owner, actual update will be delayed by SOURCE_UPDATE_DELAY
+ * @param newSource The new JavaScript source code for Chainlink Functions
+ */
+ function proposeSourceUpdate(string memory newSource) external onlyOwner {
+ pendingSource = newSource;
+
+ // Schedule the source update through the timelock
+ bytes memory data = abi.encodeWithSelector(
+ this.executeSourceUpdate.selector,
+ newSource
+ );
+
+ timelock.schedule(
+ address(this),
+ 0, // No value sent
+ data,
+ bytes32(0), // No predecessor
+ bytes32(uint256(keccak256(bytes(newSource)))), // Salt based on source
+ SOURCE_UPDATE_DELAY
+ );
+
+ emit SourceUpdateProposed(newSource);
+ }
+
+ /**
+ * @notice Executes the source code update after timelock delay
+ * @dev Only callable by the timelock contract
+ * @param newSource The new JavaScript source code
+ */
+ function executeSourceUpdate(string memory newSource) external {
+ require(msg.sender == address(timelock), "Only timelock can execute");
+ require(keccak256(bytes(pendingSource)) == keccak256(bytes(newSource)), "Source mismatch");
+
+ s_functionsConfig.source = newSource;
+ delete pendingSource;
+
+ emit SourceUpdated(newSource);
+ }
+
+ /**
+ * @notice Cancels a pending source update
+ * @dev Only callable by owner
+ * @param newSource The pending source that should be cancelled
+ */
+ function cancelSourceUpdate(string memory newSource) external onlyOwner {
+ bytes memory data = abi.encodeWithSelector(
+ this.executeSourceUpdate.selector,
+ newSource
+ );
+
+ timelock.cancel(
+ bytes32(uint256(keccak256(abi.encodePacked(
+ address(this),
+ 0,
+ data,
+ bytes32(0),
+ bytes32(uint256(keccak256(bytes(newSource))))
+ ))))
+ );
+
+ delete pendingSource;
+ emit SourceUpdateCancelled(newSource);
+ }
+
+ event SourceUpdateProposed(string newSource);
+ event SourceUpdated(string newSource);
+ event SourceUpdateCancelled(string newSource);
}
