Incomplete Function Override Allows ERC4626 Withdraw to Bypass Competition Lock-In Restrictions
Description
-
The briVault.sol contract locks user funds during the competition event. Users can only withdraw after the event ends and a winner is set, and only if they selected the winning country. This ensures participants remain committed until the competition concludes.
-
The contract inherits from ERC4626.sol, which exposes a public withdraw(uint256 assets, address receiver, address owner) function. Since briVault.sol implements a custom withdraw() function with a different signature (no parameters), it does not override the ERC4626 function. Users can call the ERC4626 withdraw function directly during the event, bypassing all restrictions (winner check, event end check, winner validation) and allowing them to exit the competition and withdraw funds at any time, including just before the event ends.
abstract contract ERC4626 is ERC20, IERC4626 {
@> function withdraw(uint256 assets, address receiver, address owner) public virtual returns (uint256) {
uint256 maxAssets = maxWithdraw(owner);
if (assets > maxAssets) {
revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
}
uint256 shares = previewWithdraw(assets);
_withdraw(_msgSender(), receiver, owner, assets, shares);
return shares;
}
}
Risk
Likelihood:
-
The ERC4626::withdraw(uint256, address, address) function remains accessible because briVault.sol inherits from ERC4626.sol and does not override it.
-
Users can call this function at any time during the event period, including just before the event ends making it extremely likely.
Impact:
Participants can exit the competition during the event and withdraw their funds, avoiding losses when they anticipate their team will lose. This defeats the intended lock-in mechanism, allowing users to game the system and withdraw funds that should remain locked until the winner is determined.
Proof of Concept
Add test_withdraw_funds_during_event_using_ERC4626_withdraw() to the briVault.t.sol test file.
function test_withdraw_funds_during_event_using_ERC4626_withdraw() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
uint256 balanceBefore = mockToken.balanceOf(user1);
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(20);
vm.stopPrank();
vm.warp(eventEndDate - 1 days);
vm.startPrank(user1);
uint256 maxWithdrawable = briVault.maxWithdraw(user1);
uint256 assetsToWithdraw = maxWithdrawable;
briVault.withdraw(assetsToWithdraw, user1, user1);
uint256 balanceAfter = mockToken.balanceOf(user1);
uint256 balanceAfterShares = briVault.balanceOf(user1);
assertGt(balanceAfter, balanceBefore, "User should have received tokens back");
assertEq(balanceAfterShares, 0, "User should have withdrawn all shares");
assertEq(balanceAfter, balanceBefore + assetsToWithdraw, "User should have received their assets back");
vm.stopPrank();
}
Recommended Mitigation
Overriding the ERC4626::maxWithdraw and ERC4626::maxRedeem would mitigate the vulnerability. This prevents the user from using the ERC4626::withdraw to bypass the intent of the protocol.
/**
* @dev Override maxWithdraw to enforce competition restrictions
* prevents withdrawals at the limit-check level
*/
+ function maxWithdraw(address owner) public view override returns (uint256) {
// Block withdrawals during event period
+ if (block.timestamp >= eventStartDate && block.timestamp < eventEndDate) {
+ return 0;
+ }
// Block withdrawals if winner not set
+ if (!_setWinner) {
+ return 0;
+ }
// Block withdrawals if user didn't win
+ if (keccak256(abi.encodePacked(userToCountry[owner])) !=
+ keccak256(abi.encodePacked(winner))) {
+ return 0;
+ }
// For winners, calculate their proportional share of finalized vault
+ uint256 ownerShares = balanceOf(owner);
+ if (ownerShares == 0 || totalWinnerShares == 0) {
+ return 0;
+ }
+ uint256 proportionalAssets = Math.mulDiv(ownerShares, finalizedVaultAsset, totalWinnerShares);
+ return proportionalAssets;
+ }
+ function maxRedeem(address owner) public view override returns (uint256) {
// Same restrictions as maxWithdraw
+ if (block.timestamp >= eventStartDate && block.timestamp < eventEndDate) {
+ return 0;
+ }
+ if (!_setWinner) {
+ return 0;
+ }
+ if (keccak256(abi.encodePacked(userToCountry[owner])) !=
+ keccak256(abi.encodePacked(winner))) {
+ return 0;
+ }
+ return balanceOf(owner);
+ }
