BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

LOW-02: Suboptimal Check Ordering in setWinner

mostafapahlevani93

Root + Impact

The setWinner() function orders validation checks inefficiently, checking expensive storage reads before cheaper ones, wasting gas on early failures.

Description

Normal behavior for gas-optimized validation expects checking the cheapest conditions first to fail fast and save gas when early checks don't pass.

The current implementation checks eventEndDate (SLOAD ~2100 gas) and array bounds before checking _setWinner flag (SLOAD ~2100 gas), when the latter should be checked first since it's the most likely to fail on repeated calls.

function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
// @> Check 1: SLOAD eventEndDate (~2100 gas)
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
// @> Check 2: Memory/calldata read (~3 gas)
require(countryIndex < teams.length, "Invalid country index");
​
// @> Check 3: SLOAD _setWinner (~2100 gas) - SHOULD BE FIRST
if (_setWinner) {
revert WinnerAlreadySet();
}
// ... rest ...
}

Risk

Likelihood:

  • Owner may accidentally call setWinner() multiple times

  • Testing environments frequently trigger this

  • Early failure case should be optimized

Impact:

  • Wasted ~2100 gas checking timestamp when winner already set

  • Wasted ~3 gas checking array bounds when winner already set

  • Total waste: ~2103 gas per failed duplicate call

  • Compounds during testing and development

  • Poor gas efficiency for common failure case

Proof of Concept

// Scenario: Owner accidentally calls setWinner twice
​
// Current implementation
vault.setWinner(0); // First call succeeds
​
vault.setWinner(0); // Second call
// Execution:
// 1. Check eventEndDate: 2100 gas (WASTED)
// 2. Check countryIndex: 3 gas (WASTED)
// 3. Check _setWinner: 2100 gas + REVERT
// Total wasted: 2103 gas before reverting
​
// ----
​
// Optimized implementation
vault.setWinner(0); // First call succeeds
​
vault.setWinner(0); // Second call
// Execution:
// 1. Check _setWinner: 2100 gas + REVERT
// Total wasted: 0 gas (fails immediately)
​
// SAVINGS: ~2100 gas per duplicate call

Recommended Mitigation

function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
+ // Check cheapest/most likely failure first
+ if (_setWinner) {
+ revert WinnerAlreadySet();
+ }
+
+ // Check parameter validity (cheap memory read)
+ if (countryIndex >= teams.length) {
+ revert InvalidCountryIndex();
+ }
+
+ // Check timestamp last (less likely to fail)
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
- require(countryIndex < teams.length, "Invalid country index");
-
- if (_setWinner) {
- revert WinnerAlreadySet();
- }
​
winnerCountryId = countryIndex;
winner = teams[countryIndex];
_setWinner = true;
​
_getWinnerShares();
_setFinallizedVaultBalance();
​
emit WinnerSet(winner);
return winner;
}

Optimization principles applied:

  1. Fail fast: Check most likely failure first

  2. Cheap first: Check cheaper operations before expensive ones

  3. State before computation: Check state flags before complex calculations

Order of operations (cheapest to most expensive):

  1. State variable read (warm): ~100 gas

  2. State variable read (cold): ~2100 gas

  3. External calls: ~2600+ gas

  4. Loop operations: Variable (potentially unlimited)

Estimated Gas Savings: ~2,100 gas per failed duplicate call (common during testing/operations)

Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Gas optimizations

Gas optimizations are invalid according to the CodeHawks documentation.

Support

FAQs

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

Give us feedback!