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

Unefficient for loop inside `BriVault::_getWinnerShares`, can lead to function reverting because out of gas

hcrlen

Unefficient for loop inside BriVault::_getWinnerShares, can lead to function reverting because out of gas

Description

  • When a users join the event, their country Id and address are assigned into userSharesToCountrymapping

  • When the event has ended, the owner can call setWinner function, this function then call _getWinnerShares

  • _getWinnerSharesreturn the totalWinnerShares

    • inside _getWinnerShares it increments userSharesToCountrymapping using a for loop, this is effective for a low number of users, but when there is too many users, looping through this mapping can cost a high number of gas

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

Risk

Owner will pay for a high amount of gas if the protocol has many users

possible for dos (denial of service) attack, if it exceeds mainnet gas limit it will revert, preventing user to withdraw their prize

Likelihood:

  • Low, we need a massive amount of users to join the tournament

Impact:

  • High, owner paying for high amount of gas (maybe exceeding the gained fee) and dos attack due to large number of users

Proof of Concept

Paste this inside the test contract

due to foundry gas limitation, we can only test for 5431 users

function test_getWinnersWillCostManyGas() public {
//owner set up the teams
vm.prank(owner);
briVault.setCountry(countries);
​
address[5431] memory users;
//loop, create 100 users, deposit, join event with random countryId
for (uint256 i = 0; i < 5431; i++) {
users[i] = makeAddr(string(abi.encodePacked("user", i)));
//assume 1 ether for the deposit
mockToken.mint(users[i], 1 ether);
vm.startPrank(users[i]);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, users[i]);
uint256 countryId = i % countries.length;
briVault.joinEvent(countryId);
​
vm.stopPrank();
}
//warp to after event end
vm.warp(eventEndDate + 1);
//get the gas left before setWinner
uint256 gasBefore = gasleft();
console.log("Gas before setWinner:", gasBefore);
//owner set the winner
vm.startPrank(owner);
briVault.setWinner(1);
string memory winner = briVault.getWinner();
vm.stopPrank();
uint256 gasAfter = gasleft();
console.log("Gas after setWinner:", gasAfter);
​
uint256 gasUsed = gasBefore - gasAfter;
console.log("Gas used for setWinner with 5431 users:", gasUsed);
}
​
Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_getWinnersWillCostManyGas() (gas: 858207616)
Logs:
Gas before setWinner: 18566070
Gas after setWinner: 963660
Gas used for setWinner with 100 users: 17602410
//0.01760241 eth
​
​

Recommended Mitigation

1. Calculate the shares whenever a user joins the event
2. Batch shares computation (divide it into several batch)
3. calculate it offchain
4. limit the number of participants/users joining the event
Updates

Appeal created

bryanconquer 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!