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

Unrestricted ERC4626 Redeem Lets Losers Drain Prize Pool

galer ah

Root + Impact

Description

BriVault inherits OpenZeppelin’s ERC4626 implementation (src/briVault.sol:13) but only overrides the custom deposit. The inherited withdraw/redeem entry points remain callable without the winner lifecycle gates. Once a losing bettor knows the outcome, they can invoke redeem(shares, loser, loser) to reclaim their stake while the vault still expects them to stay locked for payout calculations.

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

Risk

Likelihood: High – ERC4626 redeem is a standard public function that any motivated user or bot can call once the result is known.

Impact:

  • Winners recover only their own principal; the prize pool collapses.

  • Multiple loser exits can drain almost the entire vault before finalization.

Proof of Concept

  1. Two bettors deposit equal stakes via deposit and join different teams.

  2. Advance time beyond eventEndDate without calling setWinner.

  3. The future loser calls briVault.redeem(briVault.balanceOf(loser), loser, loser).

  4. Owner finalizes with setWinner(winningTeam).

  5. Winning bettor calls withdraw() and receives merely their net stake.
    (Reproduced in test/BriVaultAttack.t.sol:75 through testLoserRedeemBreaksPayout.)

function testLoserRedeemBreaksPayout() public {
vm.startPrank(loser);
uint256 loserShares = briVault.balanceOf(loser);
briVault.redeem(loserShares, loser, loser);
vm.stopPrank();
​
vm.prank(owner);
briVault.setWinner(0);
​
vm.prank(winner);
briVault.withdraw(); // yields only the winner's own stake
}

Recommended Mitigation

  1. Override ERC4626 withdraw and redeem to enforce the same lifecycle and winner checks as the bespoke withdraw(), or disable them entirely.

  2. Snapshot vault assets for payouts only after guarding against ERC4626 exits, or compute fresh balances during withdrawal.

  3. Extend the test suite to cover both standard ERC4626 and custom withdrawal paths.

Proposed patch (Solidity-like pseudocode):

function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256) {
- return super.withdraw(assets, receiver, owner);
+ revert ERC4626Disabled();
}
​
function redeem(uint256 shares, address receiver, address owner) public override returns (uint256) {
- return super.redeem(shares, receiver, owner);
+ revert ERC4626Disabled();
}
​
function withdraw() external winnerSet {
@@
- uint256 vaultAsset = finalizedVaultAsset;
- uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
+ uint256 assetToWithdraw = Math.mulDiv(
+ shares,
+ IERC20(asset()).balanceOf(address(this)),
+ totalWinnerShares
+ );
Updates

Appeal created

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