Submission Details
Severity:
No refund mechanism WHEN function fails leading to loss of funds
No Refund on (Chainlink) function failure, leads to loss of funds deposited
Description
-
When a user tries to mint the NFT, the conditional check in
fulfillMintRequestfunction ensures that it ONLY proceeds if a valid response has been received and there are no errors, but -
It is now observed that there's no mechanism to revert/refund users when the external call fails, subsequently leading to loss of funds (paid upfront).
if (response.length == 0 || err.length > 0) {
return;
}
Risk
Likelihood:
Recurrent issue, any time a user tries to mint an NFT
Impact:
The user does NOT receive an NFT
The user’s ETH is NOT refunded
The contract keeps the ETH/Link
Proof of Concept
// SPDX-License-Identifier: MIT
import {Test} from "forge-std/Test.sol";
import {WeatherNft, WeatherNftStore} from "src/WeatherNft.sol";
// Refactor to a helper contract -
contract MockFunctionsRouter {
function sendRequest(
uint64, // subId
bytes calldata, // data
uint16, // dataVersion
uint32, // callbackGasLimit
bytes32 // donId
) external payable returns (bytes32) {
return keccak256(abi.encodePacked(msg.sender, block.timestamp));
}
}
contract WeatherNftRefundVulnerabilityTest is Test {
WeatherNft public weatherNft;
address public user = address(0xBEEF);
uint256 public constant INITIAL_MINT_PRICE = 0.1 ether;
uint256 public constant STEP_INCREASE = 0.01 ether;
function setUp() public {
WeatherNftStore.Weather[] memory weathers = new WeatherNftStore.Weather[](1);
weathers[0] = WeatherNftStore.Weather.SUNNY;
string[] memory weatherURIs = new string[](1);
weatherURIs[0] = "ipfs://weather-sunny";
MockFunctionsRouter mockRouter = new MockFunctionsRouter();
address functionsRouter = address(mockRouter);
WeatherNftStore.FunctionsConfig memory config = WeatherNftStore.FunctionsConfig({
source: "source",
encryptedSecretsURL: "",
subId: 1,
gasLimit: 500000,
donId: bytes32(0)
});
address link = address(0x1002);
address keeperRegistry = address(0x1003);
address keeperRegistrar = address(0x1004);
uint32 upkeepGaslimit = 500000;
weatherNft = new WeatherNft(
weathers,
weatherURIs,
functionsRouter,
config,
INITIAL_MINT_PRICE,
STEP_INCREASE,
link,
keeperRegistry,
keeperRegistrar,
upkeepGaslimit
);
vm.deal(user, 10 ether);
}
function testSingleUserEthLossOnFunctionFailure() public {
uint256 userInitialBalance = user.balance;
uint256 contractInitialBalance = address(weatherNft).balance;
vm.prank(user);
bytes32 requestId = weatherNft.requestMintWeatherNFT{value: INITIAL_MINT_PRICE}("12345", "US", false, 3600, 0);
assertEq(user.balance, userInitialBalance - INITIAL_MINT_PRICE);
assertEq(address(weatherNft).balance, contractInitialBalance + INITIAL_MINT_PRICE);
// Simulate a failure in the fulfillRequest function - though a new external function was created in parent class
// to handle this -> excerpt from the original code below
// function testFulfillRequest(bytes32 requestId, bytes memory response, bytes memory err) external {
// fulfillRequest(requestId, response, err);
// }
weatherNft.testFulfillRequest(requestId, "", "API Error: Failed to fetch weather data");
weatherNft.fulfillMintRequest(requestId);
uint256 userFinalBalance = user.balance;
uint256 contractFinalBalance = address(weatherNft).balance;
assertEq(userFinalBalance, userInitialBalance - INITIAL_MINT_PRICE, "User lost ETH");
assertEq(contractFinalBalance, contractInitialBalance + INITIAL_MINT_PRICE, "Contract keeps ETH");
assertEq(weatherNft.s_tokenCounter(), 1, "No NFT should be minted on error (tokenCounter increments on mint)");
}
function testSuccessfulRequestWorksCorrectly() public {
uint256 userInitialBalance = user.balance;
uint256 initialTokenCounter = weatherNft.s_tokenCounter();
vm.prank(user);
bytes32 requestId = weatherNft.requestMintWeatherNFT{value: INITIAL_MINT_PRICE}("12345", "US", false, 3600, 0);
bytes memory successResponse = abi.encode(uint8(1));
weatherNft.testFulfillRequest(requestId, successResponse, "");
weatherNft.fulfillMintRequest(requestId);
assertEq(weatherNft.s_tokenCounter(), initialTokenCounter + 1, "Token counter should increment");
assertEq(user.balance, userInitialBalance - INITIAL_MINT_PRICE, "User paid for successful mint");
}
}
Recommended Mitigation
- if (response.length == 0 || err.length > 0) {
- // No handler for REFUND!
- return;
- }
+ if (response.length == 0 || err.length > 0) {
+ address user = s_funcReqIdToUserMintReq[requestId].user;
+
+ // Refund the amount the user actually paid (current price BEFORE increment)
+ uint256 refundAmount = s_currentMintPrice - s_stepIncreasePerMint;
+
+ (bool sent, ) = user.call{value: refundAmount}("");
+ require(sent, "Refund failed");
+
+ // Reset the mint price for the failed request
+ s_currentMintPrice -= s_stepIncreasePerMint;
+
+ return;
+ }
Updates
Appeal created
bube 4 months ago
Submission Judgement Published Assigned finding tags:
Lost fee in case of Oracle failure
If Oracle fails, the `fulfillMintRequest` function will not return the payed fee for the token to the user.
Rewards breakdown
nSLOC
236This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.