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

totalWinnerShares is never reset before recomputation, leading to inflated winner share totals

eze001

Root + Impact

Description

  • Normally, when computing total shares for winners, totalWinnerShares should represent the sum of current winners’ shares only once per finalization.

  • The _getWinnerShares() function keeps adding to totalWinnerShares every time it’s called, without resetting it to zero. This means that multiple invocations — either intentionally or via setWinner() or later calls — will keep increasing the total beyond its real value.

// Root cause in the codebase with @> marks to highlight the relevant section
function _getWinnerShares () internal returns (uint256) {
@> for (uint256 i = 0; i < usersAddress.length; ++i){
@> address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
@> }
return totalWinnerShares;
}
​

Risk

Likelihood:

  • Occurs whenever _getWinnerShares() is called more than once (e.g., if setWinner() is called again by mistake or another internal function reuses it).

  • Can also occur during testing or maintenance where the function is re-run to validate totals, unintentionally inflating results.

Impact:

  • Impact 1: Inflated totalWinnerShares causes underpayment to winners, since each winner’s share is calculated as Math.mulDiv(shares, vaultAsset, totalWinnerShares) — with a higher denominator reducing payouts.

  • Impact 2: Repeated calls create inconsistent accounting and possible loss of trust in payout correctness, especially if administrative or off-chain scripts trigger this function multiple times.

Proof of Concept

Observed Effect:
After two calls, totalWinnerShares becomes roughly 2× its correct value, skewing winner withdrawal amounts.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
contract PoC_DuplicateWinnerShares {
BriVault public vault;
​
constructor(BriVault _vault) { vault = _vault; }
​
function callGetWinnerSharesTwice() external {
// First call calculates correct total
vault._getWinnerShares();
// Second call adds the same user shares again
vault._getWinnerShares(); // totalWinnerShares doubles
}
}
​

Recommended Mitigation

Explanation: Resetting totalWinnerShares at the start of _getWinnerShares() ensures the total reflects the current, accurate state of all winner shares. Without this, repeated or unintended calls will keep compounding totals. This fix maintains consistent and fair reward calculation.

function _getWinnerShares () internal returns (uint256) {
- for (uint256 i = 0; i < usersAddress.length; ++i){
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
- }
- return totalWinnerShares;
+ // Reset total before recomputation to avoid double counting
+ totalWinnerShares = 0;
+ 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!