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

Stale participant records after cancellation due to missing cleanup `usersAddress`, inaccurate participant count .

chain__warden

Stale participant records after cancellation due to missing cleanup usersAddress, inaccurate participant count .

Description

  • When a user calls cancelParticipation(), the contract correctly refunds the staked amount and burns the user's shares, but it fails to remove the user's address from the usersAddress array and does not decrement the numberOfParticipants counter. As a result, the contract keeps stale participant records even though the user is no longer active.

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:

  • It's highly probable to happen.

Impact:

  • the contract state not be handled appropriately.

Proof of Concept

Put this test in `briVault.t.sol`
​
<details>
<summary>POC</summary>
​
```javascript
​
​
function test_CancelDoesNotDecreaseParticipantsArrayOrCount() public {
// Owner sets countries
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
// Five users deposit and join different teams
address[5] memory users = [user1, user2, user3, user4, user5];
uint256 amount = 10 ether;
uint256[5] memory teamsIds = [uint256(0), 1, 2, 3, 4];
​
for (uint256 i = 0; i < users.length; i++) {
vm.startPrank(users[i]);
mockToken.approve(address(briVault), amount);
briVault.deposit(amount, users[i]);
briVault.joinEvent(teamsIds[i]);
vm.stopPrank();
}
​
// Record numberOfParticipants before cancel
uint256 participantsBefore = briVault.numberOfParticipants();
​
// Compute usersAddress length before cancel by probing the public getter
uint256 arrayLenBefore = 0;
for (uint256 i = 0; ; i++) {
try briVault.usersAddress(i) returns (address addr) {
// if getter succeeds, increment length
arrayLenBefore++;
} catch {
// out of bounds -> stop
break;
}
}
​
// user5 cancels participation
vm.startPrank(user5);
briVault.cancelParticipation();
vm.stopPrank();
​
// Record numberOfParticipants after cancel
uint256 participantsAfter = briVault.numberOfParticipants();
​
// Compute usersAddress length after cancel
uint256 arrayLenAfter = 0;
for (uint256 i = 0; ; i++) {
try briVault.usersAddress(i) returns (address addr) {
arrayLenAfter++;
} catch {
break;
}
}
​
// The bug is present if the counts did NOT decrease after cancel
assertEq(participantsBefore, participantsAfter, "numberOfParticipants changed (expected unchanged)");
assertEq(arrayLenBefore, arrayLenAfter, "usersAddress length changed (expected unchanged)");
​
// optional logs for visibility
console.log("participantsBefore:", participantsBefore);
console.log("participantsAfter :", participantsAfter);
console.log("arrayLenBefore :", arrayLenBefore);
console.log("arrayLenAfter :", arrayLenAfter);
}
​
```
​
</details>
​

Recommended Mitigation

1. Add this mapping to contract.
​
`mapping(address => uint256) private userIndex;`
​
2. Add this to `briVault::joinEvent`
​
`userIndex[msg.sender] = usersAddress.length;`
​
3. Modify `briVault::cancelParticipation`
​
```diff
​
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);
​
+ uint256 idx = userIndex[msg.sender];
+ uint256 lastIndex = usersAddress.length - 1;
​
+ if (usersAddress.length > 0 && idx < usersAddress.length) {
+ if (idx != lastIndex) {
+ address lastUser = usersAddress[lastIndex];
+ usersAddress[idx] = lastUser;
+ userIndex[lastUser] = idx;
+ }
+ usersAddress.pop();
+ }
​
+ delete userIndex[msg.sender];
​
+ if (numberOfParticipants > 0) {
+ numberOfParticipants--;
+ }
+ }
​
```
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!