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

_getWinnerShares() Accumulates Without Reset Leading to Inflated Denominator

objectplayer

Root + Impact

The winner share calculation accumulates values without resetting the state variable, causing massively inflated denominators and fund loss for winners.

Description

  • The _getWinnerShares() function should calculate the total shares held by users who selected the winning country.

  • The function accumulates values into the state variable totalWinnerShares using += but never resets it to zero before calculation. If called multiple times (or even once with existing state), it will keep adding to the previous value.

function _getWinnerShares () internal returns (uint256) {
// @> totalWinnerShares is NEVER reset to 0
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood:

  • Function is called every time setWinner() is executed

  • State variable persists between calls

  • Design flaw that will manifest if contract logic changes

Impact:

  • totalWinnerShares becomes massively inflated

  • Winners receive far less than they deserve during withdrawal

  • Withdrawal calculation uses this as denominator

  • Direct fund loss for all winners

Proof of Concept

If contract is updated and the new method call _getWinnerShares() as from this name, it only returns the WinnerShares but it actually updating the state, without setting it to 0 initially

// User1 has 500 shares, User2 has 500 shares on winning country
// Total should be 1000 shares
​
// First call
vault.setWinner(0);
// totalWinnerShares = 0 + 500 + 500 = 1000 ✓
​
// If called again (hypothetically):
// totalWinnerShares = 1000 + 500 + 500 = 2000 ✗
​
// Vault has 100,000 tokens
// User1 withdrawal:
// Expected: (500 * 100000) / 1000 = 50,000 tokens
// Actual: (500 * 100000) / 2000 = 25,000 tokens
// User loses 50% of winnings!

Recommended Mitigation

This can be fixed by just setting the state to 0 initially

function _getWinnerShares () internal returns (uint256) {
+ totalWinnerShares = 0; // Reset before calculation
​
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
Updates

Appeal created

bube Lead Judge 19 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.

Give us feedback!