Location: _getWinnerShares()
Issue: Function increments totalWinnerShares on each call and never resets it. Because it is internal (called from setWinner() and possibly elsewhere), repeated invocations will keep adding the same per-user shares, causing totalWinnerShares to grow linearly with number of calls. This directly corrupts payout math in withdraw().
Vulnerable code
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
Impact
Wrong payout proportions: winners receive incorrect amounts (could be inflated/deflated).
Attacker (or re-entry via internal call path) can cause repeated calls to manipulate totalWinnerShares before final payouts.
withdraw() computes Math.mulDiv(shares, vaultAsset, totalWinnerShares) — corrupted denominator leads to incorrect token distributions or division by zero (if initial value was 0 and then set wrong elsewhere).
Proof of Concept
Deploy contract, have participants deposit and join a country so userSharesToCountry[...] contains nonzero values.
Call setWinner() once — totalWinnerShares = S.
If there is any path to call _getWinnerShares() again (e.g. accidental internal call or owner re-invoking setWinner() due to bug), totalWinnerShares becomes 2*S.
Result: all subsequent withdrawals use corrupted totalWinnerShares.
Recommended Mitigation
Compute totals in a local variable, then assign once to storage (or make the function view and return the computed value without writing to storage). Example patch:
-function _getWinnerShares () internal returns (uint256) {
- for (uint256 i = 0; i < usersAddress.length; ++i){
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
- }
- return totalWinnerShares;
-}
+function _getWinnerShares () internal view returns (uint256) {
+ uint256 total = 0;
+ for (uint256 i = 0; i < usersAddress.length; ++i) {
+ address user = usersAddress[i];
+ total += userSharesToCountry[user][winnerCountryId];
+ }
+ return total;
+}
Then in setWinner():
- _getWinnerShares();
-
- _setFinallizedVaultBalance();
+ // compute once and assign
+ totalWinnerShares = _getWinnerShares();
+ finalizedVaultAsset = _setFinallizedVaultBalance();
