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

Unbounded iteration over `usersAddress` array in `briVault::_getWinnerShares`, Lead to probability of failure `briVault::setWinner` because of Gas-DoS that makes winner finalization and winners' withdrawals fail and Freezing funds within contract

chain__warden

Unbounded iteration over usersAddress array in briVault::_getWinnerShares, Lead to probability of failure briVault::setWinner because of Gas-DoS that makes winner finalization and winners' withdrawals fail and Freezing funds within contract.

Description

  • There is a high probability that the briVault::setWinner function call will fail because it calls the briVault::_getWinnerShares function, which loops the users array. The larger the number of users, the more expensive this loop becomes, ultimately preventing the winner from being set and preventing the winning users from withdrawing funds, and Freezing funds within the contract.

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:

  • It might occur under specific conditions. For example, A significant increase in the number of participants, or an attacker's attempt to fill the array with users by making a single deposit and calling the joinEvent function a very large number of times.


Impact:

  • preventing the winner from being set and preventing the winning users from withdrawing funds, and Freezing funds within the contract.


Proof of Concept

In the following test, we try three scenarios, once with 100, 500, and 1000 users, and calculate the gas cost of the `briVault::setWinner` function each time. We find that it increases with every increase in the number of users in the array.
​
Put this test in `briVault.t.sol`
​
<details>
<summary>POC</summary>
​
function test_DOS_in_getWinnerShares_with_large_users() public {
uint256[3] memory sizes = [uint256(100), uint256(500), uint256(1000)];
uint256 depositAmount = 0.0003 ether;
uint256 countryId = 10;
​
for (uint256 si = 0; si < sizes.length; si++) {
uint256 size = sizes[si];
​
// Deploy NEW vault
vm.startPrank(owner);
BriVault localVault = new BriVault(
IERC20(address(mockToken)),
participationFeeBsp,
eventStartDate,
participationFeeAddress,
minimumAmount,
eventEndDate
);
localVault.setCountry(countries);
vm.stopPrank();
​
// Move time BEFORE event start to allow deposits
vm.warp(eventStartDate - 1);
​
// Create `size` users dynamically
for (uint256 i = 0; i < size; i++) {
address u = address(uint160(uint256(keccak256(abi.encodePacked(si, i)))));
​
mockToken.mint(u, 1 ether);
​
vm.startPrank(u);
mockToken.approve(address(localVault), depositAmount);
// deposit
try localVault.deposit(depositAmount, u) {} catch {
vm.stopPrank();
continue;
}
// joinEvent
try localVault.joinEvent(countryId) {} catch {
vm.stopPrank();
continue;
}
vm.stopPrank();
}
​
// After event end → setWinner
vm.warp(eventEndDate + 1);
​
vm.startPrank(owner);
uint256 gasBefore = gasleft();
(bool success, ) = address(localVault).call(
abi.encodeWithSignature("setWinner(uint256)", countryId)
);
uint256 gasUsed = gasBefore - gasleft();
vm.stopPrank();
​
if (success) {
console.log("setWinner SUCCESS for size", size);
console.log("Gas used:", gasUsed);
} else {
console.log("setWinner REVERTED for size", size);
console.log("Gas before revert:", gasUsed);
}
}
}

Recommended Mitigation

1. Add a variable to aggregate each country's shares during accession.
​
2. In `brivault::joinEvent` update the total number of Country shares directly.
​
3. Make `brivault::_getWinnerShares` without any loop.
​
```diff
​
+ mapping(uint256 => uint256) public totalSharesPerCountry;
​
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
​
// Ensure countryId is a valid index in the `teams` array
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;
+ totalSharesPerCountry[countryId] += participantShares;
​
usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
```
​
```diff
​
function _getWinnerShares () internal returns (uint256) {
​
- for (uint256 i = 0; i < usersAddress.length; ++i){
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
​
+ totalWinnerShares = totalSharesPerCountry[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!