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

ERC4626 Standard Compliance - withdraw() Function Signature Doesn't Match Standard

ke5haav

Root + Impact

Description

  • The contract inherits from ERC4626, which implements the ERC4626 Tokenized Vault Standard. Under normal behavior, all functions should comply with the standard interface, allowing the contract to integrate seamlessly with DeFi protocols, aggregators, and other systems that expect ERC4626-compliant vaults

  • However, the contract overrides the withdraw() function on line 294 with a custom signature that doesn't match the ERC4626 standard. The standard requires withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares), but this contract implements withdraw() external winnerSet with no parameters. This breaks ERC4626 compliance, preventing the contract from being used with protocols that rely on the standard interface, such as yield aggregators, lending protocols, or other DeFi integrations that expect the standard withdraw() signature.

// Standard ERC4626 signature (from IERC4626.sol line 188)
function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares);
​
// This contract's implementation (line 294)
@>function withdraw() external winnerSet {
// ... custom implementation with no parameters
}

Risk

Likelihood

  • This non-compliance occurs whenever external protocols or integrations attempt to call the standard ERC4626 withdraw() function, as the function signature mismatch will cause compilation errors or runtime failures

  • The bug manifests during integration attempts when DeFi protocols, aggregators, or other smart contracts try to interact with this vault using the standard ERC4626 interface

Impact

  • The contract cannot be integrated with DeFi protocols that expect ERC4626-compliant vaults, limiting its usability and preventing composability

  • External systems that rely on the standard interface will fail when attempting to interact with this vault, potentially causing integration failures and user confusion

Proof of Concept

function testERC4626Compliance() public {
// Standard ERC4626 interface expects:
// function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares);
// This contract has:
// function withdraw() external winnerSet;
// Verify interface mismatch
bytes4 standardSelector = IERC4626.withdraw.selector;
bytes4 contractSelector = bytes4(keccak256("withdraw()"));
assertNotEq(standardSelector, contractSelector, "Function selectors don't match");
// Try to call standard interface - this will fail
IERC4626 vault = IERC4626(address(briVault));
(bool callSuccess,) = address(vault).call(
abi.encodeWithSelector(IERC4626.withdraw.selector, 1 ether, address(this), address(this))
);
assertFalse(callSuccess, "Standard withdraw call should fail");
}

Explanation of PoC:

This proof of concept demonstrates the non-compliance by showing that the contract's withdraw() function doesn't match the ERC4626 standard signature. The test verifies the function selector mismatch and shows that the standard interface cannot be called.

Test Results:

  • ✅ Contract's withdraw() has different signature than standard

  • ✅ Cannot be called with standard ERC4626 interface parameters

  • ✅ Breaks interface compliance

Recommended Mitigation

Explanation:

The recommended mitigation implements the standard ERC4626 withdraw() function signature while maintaining the custom winner verification logic. This ensures ERC4626 compliance while preserving the winner verification functionality.

Key Changes:

  1. Implement the standard withdraw(uint256 assets, address receiver, address owner) function signature

  2. Add winner verification logic inside the standard function

  3. Maintain backward compatibility by keeping the custom function that calls the standard one

+ function withdraw() external winnerSet {
+ // Backward compatibility wrapper
+ withdraw(balanceOf(msg.sender), msg.sender, msg.sender);
+ }
+
+ function withdraw(uint256 assets, address receiver, address owner)
+ public
+ override
+ winnerSet
+ returns (uint256 shares)
+ {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
​
if (
- keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
+ keccak256(abi.encodePacked(userToCountry[owner])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
- uint256 shares = balanceOf(msg.sender);
+ shares = userSharesToCountry[owner][winnerCountryId];
+ if (shares == 0) revert didNotWin();
+
uint256 vaultAsset = finalizedVaultAsset;
- uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
+ uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
+
+ if (assets > assetToWithdraw) {
+ revert ERC4626ExceededMaxWithdraw(owner, assets, assetToWithdraw);
+ }
_burn(owner, shares);
- IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
- emit Withdraw(msg.sender, assetToWithdraw);
+ IERC20(asset()).safeTransfer(receiver, assetToWithdraw);
+ emit Withdraw(msg.sender, receiver, owner, assetToWithdraw, shares);
+
+ return shares;
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!