BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: medium
Invalid

Non-Compliant withdraw Function Breaks ERC-4626 Composability

0xvrka

Root + Impact

Description

  • The ERC-4626 standard requires that withdraw and redeem functions support a flow where a msg.sender (with an allowance) can act on behalf of an owner. This is the entire purpose of the standard, as it allows other protocols (like yield aggregators, dashboards, or "claim bots") to manage user positions.

  • The contract's custom withdraw() function (the only way to claim winnings) is hard-coded to msg.sender. It checks userToCountry[msg.sender], gets balanceOf(msg.sender), and pays msg.sender. It is impossible for another contract to call this function on behalf of a winner, even with a full share allowance, completely breaking composability.

// The ERC-4626 Standard that is NOT being met:
// "MUST support a withdraw flow where the shares are burned from owner directly
// where msg.sender has EIP-20 approval over the shares of owner."
​
function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
@> if (
@> keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
@> keccak256(abi.encodePacked(winner))
@> ) {
@> revert didNotWin();
@> }
@> uint256 shares = balanceOf(msg.sender);
​
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
@> _burn(msg.sender, shares);
@> IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
@> emit Withdraw(msg.sender, assetToWithdraw); // (Also non-compliant event)
}

Risk

Likelihood:

  • This is a fundamental design flaw, not an edge-case bug. The feature is 100% missing.

  • Any external protocol (e.g., a "claim-all" dashboard, a re-staking protocol, a yield manager) attempting to integrate with this vault will fail.

  • It's not a malicious attack but a guaranteed failure of the contract's core promise (being ERC-4626 compliant).

Impact:

  • The contract is unusable by any other DeFi protocol. Users cannot have their winnings claimed by automated services, moved by yield aggregators, or managed by any third-party contract, which defeats the entire purpose of using the ERC-4626 standard.

Proof of Concept

A conceptual PoC:

  1. user1 (a winner) has 100 shares.

  2. user1 calls briVault.approve(address(claimBot), 100) to let a bot claim their winnings.

  3. claimBot (as msg.sender) calls briVault.withdraw().

  4. The call reverts at the didNotWin() check, because userToCountry[address(claimBot)] is not set. The claimBot has no way to specify it is acting on behalf of user1.

Recommended Mitigation

Remove your custom withdraw() function entirely and replace it with the standard, overridden ERC-4626 withdraw function. Add your custom winner-checking logic inside it.

- // REMOVE THIS ENTIRE CUSTOM FUNCTION
- function withdraw() external winnerSet {
- if (block.timestamp < eventEndDate) {
- revert eventNotEnded();
- }
- if (
- keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
- keccak256(abi.encodePacked(winner))
- ) {
- revert didNotWin();
- }
- uint256 shares = balanceOf(msg.sender);
- uint256 vaultAsset = finalizedVaultAsset;
- uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
- _burn(msg.sender, shares);
- IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
- emit Withdraw(msg.sender, assetToWithdraw); // (Also non-compliant event)
- }
​
​
+ /**
+ * @dev Overrides the standard ERC4626 withdraw function to add event logic.
+ * This is now the *only* way to claim winnings.
+ * Note: This function takes ASSETS as input, not shares.
+ */
+ function withdraw(
+ uint256 assets,
+ address receiver,
+ address owner
+ ) public override winnerSet returns (uint256 shares) {
+ // Check if the owner (the person whose shares are being burned) is a winner
+ if (
+ keccak256(abi.encodePacked(userToCountry[owner])) !=
+ keccak256(abi.encodePacked(winner))
+ ) {
+ revert didNotWin();
+ }
+
+ // Calculate the shares needed to get 'assets'
+ uint256 vaultAsset = finalizedVaultAsset;
+ // Rounding UP to ensure user gets at least 'assets'
+ shares = Math.mulDiv(assets, totalWinnerShares, vaultAsset, Math.Rounding.Up);
+
+ // fixing share transfering to guarantee win
+ uint256 sharesSnapshot = userSharesToCountry[owner][winnerCountryId];
+ if (shares > sharesSnapshot) revert("Exceeds share snapshot");
+ userSharesToCountry[owner][winnerCountryId] -= shares;
+
+ // Use the inherited _withdraw to properly check allowance and burn shares
+ // This will burn 'shares' from 'owner' and send 'assets' to 'receiver'
+ _withdraw(owner, shares, assets, receiver);
+
+ // The _withdraw function already emits the standard ERC-4626 Withdraw event
+ return shares;
+ }
Updates

Appeal created

bube Lead Judge 16 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!