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

Potential Denial of Service due to an unbounded loop in `setWinner`

darkfox

Description

When the eventEndDate has passed, the owner is expected to call setWinner, which calculates the total number of shares for the winning team.

However, _getWinnerShares, which is called within setWinner, iterates over the entire userAddresses array to compute totalWinnerShares. This loop is unbounded, meaning that gas usage grows linearly with the number of users. As the array size increases, the cost of executing setWinner also increases.

If userAddresses becomes too large, the gas cost may exceed the block gas limit, causing the transaction to revert and preventing the owner from finalizing the event.

function _getWinnerShares () internal returns (uint256) {
​
@> for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}
​
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;
​
@> usersAddress.push(msg.sender);
​
numberOfParticipants++;
totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}

Risk

Likelihood:

Every call to joinEvent appends the user’s address to userAddresses, even if the same user calls joinEvent multiple times. As a result, the array can grow very large with a sufficient amount of users.

Impact:

The owner may be unable to execute setWinner due to excessive gas costs, effectively freezing all vault assets and preventing users from claiming rewards. This results in a complete denial of service for the protocol’s core functionality.

Proof of Concept

Add this test to your test suite in test/briVault.t.sol.

function testSetWinnerDOSWhenThereAreManyUsers() public {
vm.prank(owner);
briVault.setCountry(countries);
​
uint256 depositAmount = 0.005 ether;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
briVault.deposit(depositAmount, user1);
for (uint256 i = 0; i < 30_000; i++) { briVault.joinEvent(i % countries.length); }
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
​
bytes memory data = abi.encodeWithSelector(BriVault.setWinner.selector, 0);
vm.prank(owner);
uint256 g0 = gasleft();
(bool ok, ) = address(briVault).call{gas: 30_000_000}(data);
uint256 used = g0 - gasleft();
​
assert(!ok);
console.log("setWinner gas used for 30k entries in userAddresses:", used);
}

This test shows that a malicious user can inflate userAddresses to 30,000 entries. At this size, setWinner consumes over 30 million gas, which is around Ethereum’s block gas limit, causing it to revert or become very expensive for the owner.

Recommended Mitigation

Use a mapping to track total team shares instead of iterating through an array of user addresses.

​
- mapping(address => mapping(uint256 => uint256)) public userSharesToCountry;
+ mapping(uint256 => uint256) public countryIdToShares;
​
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;
+ countryIdToShares[countryId] += participantShares;
​
- usersAddress.push(msg.sender);
​
- numberOfParticipants++;
- totalParticipantShares += participantShares;
​
emit joinedEvent(msg.sender, countryId);
}
​
- function _getWinnerShares () internal returns (uint256) {
+ function _getWinnerShares (uint256 countryIndex) internal returns (uint256) {
​
- for (uint256 i = 0; i < usersAddress.length; ++i){
- address user = usersAddress[i];
- totalWinnerShares += userSharesToCountry[user][winnerCountryId];
- }
​
+ totalWinnerShares = countryIdToShares[countryIndex];
​
return totalWinnerShares;
}

This change removes the need for an unbounded loop and ensures that the gas cost of setWinner remains constant, regardless of the number of participants.

Additional changes would need to be made to prevent multiple calls to join a team by the same user and account for multiple deposits.

Updates

Appeal created

bube Lead Judge 20 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!