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

Canceling participation locks funds inside the protocol.

nkh000

When a user cancels his participation, user address from the userAddresses array is not removed and userSharesToCountry is not set to zero resulting in funds being locked in the protocol.

Description

When user calls briVault::cancelParticipation than at no point in the entire function the user address from the usersAddress array which is used in calculation of totalWinnerShares in the the function briVault::_getWinnerShares is removed resulting in wrong amount calculation for reward distribution and ultimately lock funds in the protocol

  • When a user cancels their participation, then they must not be able to get the reward as well as protocol should not consider them during the reward calculation.

  • However, when user cancels there participation then their address is not removed from the usersAddress array and also not from the userSharesToCountry mapping resulting in error calculation

    // @audit not updating usersAddress array as well as userSharesToCountry mapping
    function cancelParticipation() public {
    ...
    }
    ​
    // @audit when calculating winner shares it includes every address from usersAddress array and userSharesToCountry mapping
    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

  • Happens everytime when user cancels their participation


Impact: HIGH

  • Wrong calculation of rewards

  • Funds being locked in the protocol with no way to exit

Proof of Concept

In this POC, when User1 cancels the participation, then total winning shares get influenced by his amount of shares as they were removed from the usersAddress array and userSharesToCountry mapping resulting in wrong calculation of rewards.

function test_cancelParticipationPOC() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
// user1 deposits to the vault
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
uint256 sharesOfUser1 = briVault.deposit(5 ether, user1);
vm.stopPrank();
​
// user2 deposits to the vault
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
uint256 sharesOfUser2 = briVault.deposit(5 ether, user2);
vm.stopPrank();
​
// user3 deposits to the vault
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
uint256 sharesOfUser3 = briVault.deposit(5 ether, user3);
vm.stopPrank();
​
// user1 joins the event
vm.prank(user1);
briVault.joinEvent(1);
​
// user2 joins the event
vm.prank(user2);
briVault.joinEvent(1);
​
// user3 joins the event
vm.prank(user3);
briVault.joinEvent(1);
​
uint256 vaultBalanceBeforeCancel = mockToken.balanceOf(
address(briVault)
);
​
// user1 cancel his participation to the event
vm.prank(user1);
briVault.cancelParticipation();
uint256 vaultBalanceAfterCancel = mockToken.balanceOf(
address(briVault)
);
​
// time traveling to end of the event
vm.warp(eventEndDate + 2 seconds);
vm.prank(owner);
briVault.setWinner(1);
​
// user two withdraws
vm.prank(user2);
briVault.withdraw();
​
// user3 withdraw
vm.prank(user3);
briVault.withdraw();
​
// both of them getting equal amount after withdrawals as two active winners
assertEq(
getAssetToWithdraw(sharesOfUser2),
getAssetToWithdraw(sharesOfUser3)
);
​
// ideal split of the total winning shares: vaultBalanceAfterCancel / 2 (two active winners)
uint256 idealHalf = Math.ceilDiv(vaultBalanceAfterCancel, 2);
​
// ideal half is not equal to amount withdraw as influenced by user1 shares which was never removed
assert(idealHalf != getAssetToWithdraw(sharesOfUser2));
}

Recommended Mitigation

Make following changes to the cancelParticiaption function which enforces deleting user address from the usersAddress array and shares from the userSharesToCountry mapping

function cancelParticipation() public {
...
​
// Remove user from usersAddress array
+ for (uint256 i = 0; i < usersAddress.length; ++i) {
+ if (usersAddress[i] == msg.sender) {
+ usersAddress[i] = usersAddress[usersAddress.length - 1];
+ usersAddress.pop();
+ break;
+ }
+ }
​
+ string memory country = userToCountry[msg.sender];
+ userToCountry[msg.sender] = "";
// removing the user shares to country and setting it to zero
+ for (uint256 i = 0; i < teams.length; ++i) {
+ if (
+ keccak256(abi.encodePacked(teams[i])) ==
+ keccak256(abi.encodePacked(country))
+ ) {
+ userSharesToCountry[msg.sender][i] = 0;
+ break;
+ }
+ }
​
uint256 shares = balanceOf(msg.sender);
...
}
Updates

Appeal created

bube Lead Judge 20 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!