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

MEDIUM-06: ERC4626 Standard Non-Compliance

mostafapahlevani93

Root + Impact

The contract claims to be ERC4626-compliant but only implements deposit() while missing other required functions, breaking composability with DeFi protocols.

Description

Normal behavior for an ERC4626 vault expects implementation of all standard functions: deposit(), mint(), withdraw(), redeem(), and their preview functions. This enables seamless integration with aggregators, routers, and other DeFi protocols.

The current implementation only overrides deposit() with custom logic and leaves the base ERC4626 functions potentially broken or with default behavior that doesn't match the tournament mechanics.

// @> Claims ERC4626 compliance
contract BriVault is ERC4626, Ownable {
// ... state vars ...
// @> Only overrides deposit()
function deposit(
uint256 assets,
address receiver
) public override returns (uint256) {
// Custom tournament logic
}
// @> Missing overrides:
// - withdraw(uint256 assets, address receiver, address owner)
// - redeem(uint256 shares, address receiver, address owner)
// - mint(uint256 shares, address receiver)
//
// Base implementations don't understand tournament logic
// May allow bypassing event restrictions
}

Risk

Likelihood:

  • DeFi protocols attempting to integrate will call standard ERC4626 functions

  • Base implementations may allow actions that should be restricted

  • Users familiar with ERC4626 may expect standard behavior

Impact:

  • Calling withdraw() from base contract may bypass winner checks

  • Calling redeem() may allow extraction before event ends

  • Calling mint() may allow share creation without deposits

  • Aggregators and routers will malfunction when integrating

  • Claiming ERC4626 compliance while not fully implementing it is misleading

  • Security assumptions about restricted withdrawals may be violated

  • Protocol cannot integrate with Yearn, Beefy, or other vault aggregators

Proof of Concept

// User tries to bypass tournament logic using base ERC4626 functions
​
// After depositing through custom deposit()
user.deposit(1000e18, user);
​
// Try to withdraw before event ends using base withdraw()
// Base ERC4626.withdraw() implementation:
// function withdraw(uint256 assets, address receiver, address owner) public {
// _burn(owner, shares);
// asset.transfer(receiver, assets);
// }
​
vault.withdraw(990e18, user, user);
// This might work if base implementation isn't properly overridden
// Bypasses all tournament logic and time restrictions
​
// OR
​
// Try to call redeem() to get assets back
vault.redeem(balanceOf(user), user, user);
// Again, may bypass all custom restrictions

Recommended Mitigation

Option 1: Disable base ERC4626 functions

+// Override and disable standard withdraw/redeem
+function withdraw(
+ uint256 assets,
+ address receiver,
+ address owner
+) public pure override returns (uint256) {
+ revert("Use withdraw() without parameters after event ends");
+}
+
+function redeem(
+ uint256 shares,
+ address receiver,
+ address owner
+) public pure override returns (uint256) {
+ revert("Use withdraw() without parameters after event ends");
+}
+
+function mint(
+ uint256 shares,
+ address receiver
+) public pure override returns (uint256) {
+ revert("Use deposit() function");
+}

Option 2: Don't inherit ERC4626

-contract BriVault is ERC4626, Ownable {
+contract BriVault is ERC20, Ownable {
​
// Implement custom vault logic without claiming ERC4626 compliance
// More honest about what the contract does
}

Option 3: Properly implement ERC4626 (complex)

+function withdraw(
+ uint256 assets,
+ address receiver,
+ address owner
+) public override returns (uint256) {
+ // Verify caller is owner or has allowance
+ if (msg.sender != owner) {
+ _spendAllowance(owner, msg.sender, shares);
+ }
+
+ // Check if winner is set
+ if (!_setWinner) {
+ revert winnerNotSet();
+ }
+
+ // Check if user won
+ if (userToCountryId[owner] != winnerCountryId) {
+ revert didNotWin();
+ }
+
+ // Calculate shares needed for assets
+ uint256 shares = previewWithdraw(assets);
+
+ _burn(owner, shares);
+ IERC20(asset()).safeTransfer(receiver, assets);
+
+ emit Withdraw(msg.sender, receiver, owner, assets, shares);
+ return shares;
+}
​
// Similar implementations needed for redeem(), mint()

Recommendation: Option 1 or 2 is safer. Properly implementing ERC4626 for a tournament vault is complex and may not be worth the compatibility if standard vault integrations don't make sense for this use case.

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!