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

## Iterating Over All Users in `BriVault::_getWinnerShares ()` Allows Potential DoS

mafa

Iterating Over All Users in BriVault::_getWinnerShares () Allows Potential DoS

Description

The internal function _getWinnerShares(), which is called by setWinner(), contains an unbounded loop that iterates over all entries in the usersAddress array to calculate the totalWinnerShares. As the number of users increases, the gas cost of this operation grows linearly. When the number of participants becomes large, the function may consume excessive gas and exceed the block gas limit, causing setWinner() to revert.

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

Risk

This creates a potential Denial of Service (DoS) condition, where the contract owner is unable to finalize the winner or execute other dependent operations.

Proof of Concept

  • testSetWinnerOnlyOneUser: Calculates the gas cost when only one participant joins.

  • testSetWinnerMoreUsers: Calculates the gas cost when three participants join.

Place the following code in briVault.t.sol.

Proof Of Code

function testSetWinnerOnlyOneUser() public {
// only 1 people join
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
uint256 gasbefore= gasleft();
briVault.setWinner(10);
uint256 gasAfter= gasleft();
console2.log("gas used when only 1 user", gasbefore - gasAfter);
vm.stopPrank();
}
​
function testSetWinnerMoreUsers() public {
// 1st people join
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
// 2nd
vm.startPrank(user2);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user2);
briVault.joinEvent(7);
vm.stopPrank();
​
// 3rd
vm.startPrank(user3);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user3);
briVault.joinEvent(4);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
uint256 gasbefore= gasleft();
briVault.setWinner(10);
uint256 gasAfter= gasleft();
console2.log("gas used when 3 users ", gasbefore - gasAfter);
vm.stopPrank();
}
gas used when only 1 user 103929
gas used when 3 users 110447
​

When only two additional participants join, the gas cost increases by 6,518.

Recommended Mitigation

Use an accumulated total approach instead of looping through all users. Maintain a running total of shares per country that updates whenever users join event. This removes the need for an unbounded loop and keeps gas usage constant.

+ mapping(uint256 => uint256) public CountryToTotalshares;
​
function joinEvent(uint256 countryId) public {
...
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
// update each country's total shares
+ CountryToTotalshares[countryId] += participantShares;
​
- usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
​
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!