Raisebox Faucet
First Flight #50
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: high
Invalid

Unnecessary Storage of faucetClaimer Increases Gas & Confuses State

minionteechs

Root + Impact

Description

Expected behavior

Transient per-call values (like the current caller) should be kept in memory (local variables) or emitted as events if they must be observed off-chain. They should not be written to persistent storage unless there is a functional need to keep that history.

Actual behavior

The contract writes msg.sender into storage (faucetClaimer) on every claim and exposes it via getClaimer(). This:

costs extra gas per claim (storage writes are expensive), and

exposes only the last claimer (not a history), which is unlikely to be useful.


// Root cause in the codebase with @> marks to highlight the relevant section
@> address public faucetClaimer;
​
...
​
function claimFaucetTokens() public {
@> faucetClaimer = msg.sender;
​
// ... later ...
@> _transfer(address(this), faucetClaimer, faucetDrip);
emit Claimed(msg.sender, faucetDrip);
}

Risk

Likelihood

High — the write occurs on every claim call (very frequent in faucet usage), so the gas impact is repeated.

Impact

1. Gas inefficiency: Each claim incurs an unnecessary SSTORE, increasing cost for claimers or the owner (if owner pays gas).

2. State pollution: Public state holds ephemeral data (last claimer) that provides little operational value.

3. Potential confusion: External tools might rely on getClaimer() incorrectly, assuming it represents something more persistent.

Proof of Concept

Explanation

Because faucetClaimer is only used to temporarily reference the caller inside claimFaucetTokens(), storing it in contract storage is wasteful. A local memory variable (or using msg.sender directly) achieves the same functionality without persistent SSTORE. If the team truly needs to record the last claimer, an event (e.g., event LastClaimer(address indexed who, uint256 timestamp)) is a far cheaper and more appropriate mechanism.

// Pseudocode / Foundry test outline
// 1) Deploy contract
// 2) Call claimFaucetTokens() as user A
// 3) Observe that faucet.getClaimer() == user A
// 4) Call claimFaucetTokens() as user B
// 5) Observe that faucet.getClaimer() == user B
// 6) Compare gas used for claim with and without SSTORE (local variable approach)
// -> With SSTORE: extra storage write (approx. +~5k gas per SSTORE on many EVM configs)

Recommended Mitigation

Explanation

The mitigation removes the unnecessary persistent storage of faucetClaimer and replaces it with a local variable (address faucetClaimer = msg.sender;).

This prevents an SSTORE operation during every claim, reducing gas usage significantly while maintaining the same functionality.

If tracking the last claimer is still desired for analytics or transparency, emitting an event like LastClaimer(address indexed who, uint256 timestamp) is more efficient and avoids on-chain state bloat.

- remove this code
+ add this code
- address public faucetClaimer;
+ // removed: transient caller should not be stored persistently
​
...
​
function claimFaucetTokens() public {
- faucetClaimer = msg.sender;
+ address faucetClaimer = msg.sender;
​
if (block.timestamp < (lastClaimTime[faucetClaimer] + CLAIM_COOLDOWN)) {
revert RaiseBoxFaucet_ClaimCooldownOn();
}
​
// ... use faucetClaimer (local) for interactions ...
- _transfer(address(this), faucetClaimer, faucetDrip);
+ _transfer(address(this), faucetClaimer, faucetDrip);
​
- }
+ }
Updates

Lead Judging Commences

inallhonesty Lead Judge 8 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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