BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Incorrect final vault accounting (`balanceOf`) allows asset inflation before finalization — winners can be overpaid (Inflation / payout manipulation)

xcryptoguardian

Description:

The BriVault::_getWinnerShares() function loops through the entire usersAddress array to compute the total shares of users who selected the winning country:

// @audit DoS Attack
function _getWinnerShares() internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i) {
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

This loop is unbounded, meaning its cost grows linearly with the number of participants.
The problem is that _getWinnerShares() is called inside setWinner(), which must succeed before withdrawals are allowed. If the number of participants becomes large (thousands+), this loop will exceed the block gas limit, causing setWinner() to always revert.
Since the vault relies on _setWinner == true before users can withdraw, this design allows a single attacker to force a permanent DoS simply by injecting a large number of entries into the vault.

Likelihood:

  • Reason 1: This issue occurs once the participants list grows large enough that iterating over usersAddress exceeds the block gas limit during setWinner(), making the function revert consistently.

  • Reason 2: This situation arises whenever an attacker (or organic user growth) submits a high number of deposits and joins the event repeatedly, since there is no cap or restriction on how many entries can be added.

Impact: Admin cannot call setWinner() once the participants list grows large → function becomes uncallable due to gas limit.
_setWinner never becomes true.
withdraw() requires winnerSet modifier:

modifier winnerSet() {
if (_setWinner != true) revert winnerNotSet();
_;
}

This directly leads to a High-severity Denial-of-Service where the contract cannot progress to the withdrawal phase, completely breaking core protocol functionality.

Proof of Concept:

Number of participants Gas used by setWinner()
~1000 users ≈ 0.9 – 1.1 million gas
~2000 users ≈ 1.8 – 2.0 million gas
~3000 users ≈ 2.7 – 3.0 million gas
~4000 users ≈ 3.6 – 4.0 million gas
PoC Place the following test into `brivault.t.sol`.
function test_DoS_getWinnerShares() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
uint256 numUsers = 4000; // stable upper bound
uint256 stake = 0.001 ether;
​
for (uint256 i = 0; i < numUsers; i++) {
if (i % 1000 == 0) console.log("Loop index:", i);
​
address attackerWallet = address(uint160(i + 5000));
mockToken.mint(attackerWallet, stake);
​
vm.startPrank(attackerWallet);
mockToken.approve(address(briVault), stake);
briVault.deposit(stake, attackerWallet);
briVault.joinEvent(0);
vm.stopPrank();
}
​
vm.warp(eventEndDate + 1);
​
// Sweet spot where:
// - setWinner() starts
// - cannot finish 4000-loop
// - reverts due to OOG (DoS confirmed)
uint256 forcedGas = 3_000_000;
​
vm.prank(owner);
(bool success, ) = address(briVault).call{gas: forcedGas}(
abi.encodeWithSelector(briVault.setWinner.selector, 0)
);
​
console.log("Success (should be false):", success);
​
assertFalse(
success,
"Expected DoS: setWinner() should revert due to gas exhaustion"
);
}
// [PASS] test_DoS_getWinnerShares() (gas: 613550557)
// Logs:
// Loop index: 0
// Loop index: 1000
// Loop index: 2000
// Loop index: 3000
// Success (should be false): false
​

Recommended Mitigation:

There are a few recomendations.

  1. Impose participation limits / block Sybil spam, But this alone won’t fully solve scalability issues.

  2. Use a mapping countryId → totalShares

mapping(uint256 => uint256) public totalSharesByCountry;

  1. Use pull-based withdrawal math
    Every user computes their own proportional reward without global aggregation.

[H-1] Unbounded iteration over usersAddress array in BriVault::_getWinnerShares() causes permanent Denial-of-Service (DoS), preventing winner finalization and blocking withdrawals

Description:

Likelihood:

Impact:

Proof of Concept:

Recommended Mitigation:

[H-1] Incorrect final vault accounting (balanceOf) allows asset inflation before finalization — winners can be overpaid (Inflation / payout manipulation)

Description:

BriVault._setFinallizedVaultBalance() uses the raw token balance of the contract to record the final vault assets:

function _setFinallizedVaultBalance() internal returns (uint256) {
if (block.timestamp <= eventStartDate) {
revert eventNotStarted();
}
// @audit wrong accounting (blanceOf instead of totalAssets?)
@>> return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));
}

Because finalizedVaultAsset is taken from ERC20.balanceOf(address(this)), anyone can inflate this value by transferring tokens directly to the vault before setWinner() finalises the event. withdraw() uses finalizedVaultAsset to compute user payouts:

uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);

This makes payouts manipulable by external transfers (inflation attack): attackers or benign third parties can send tokens to the vault immediately before finalization so winners receive a larger share than entitled.

Likelihood:

  • Reason 1 — The vault accepts ERC20 tokens and there is no restriction preventing arbitrary transfers to the vault address. External token transfers to the contract can occur at any time prior to finalization.

  • Reason 2 — Finalization is performed once by the owner; this single point in time is a realistic window for an attacker to cheaply deposit tokens to inflate balanceOf
    immediately before setWinner().

Impact:

  • Impact 1 — Winners receive inflated payouts: external token transfers directly increase finalizedVaultAsset, so winners can be overpaid and honest participants or protocol funds are diverted.

  • Impact 2 — Protocol economics are broken (incorrect distribution), reputation and funds at risk; if the inflation is manipulative, owner or participants may lose funds relative to intended distribution.

Proof of Concept:

High-level steps (manual / script PoC):

  1. Deploy BriVault and have several participants deposit and join the event. Example: A, B, C deposit normal amounts and join. totalWinnerShares is computed on finalization based on shares assigned earlier.

  2. Before the owner calls setWinner(...) (but after eventEndDate), attacker sends X tokens to the vault directly:

IERC20(asset).transfer(address(briVault), X);

This increases IERC20(asset).balanceOf(address(briVault)) by X.

  1. Owner calls setWinner(countryIndex). _setFinallizedVaultBalance() reads the inflated balance and records finalizedVaultAsset = original_pool + X.

  2. Winners call withdraw(); each winner receives shares / totalWinnerShares * finalizedVaultAsset, i.e. an inflated payout that includes X.

PoC Place the following test into `brivault.t.sol`.
function test_InflationAttack_finalizedVaultBalanceManipulation() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
uint256 stake = 1 ether;
​
// user1
vm.startPrank(user1);
mockToken.approve(address(briVault), stake);
uint256 u1Shares = briVault.deposit(stake, user1);
briVault.joinEvent(0);
vm.stopPrank();
​
// user2
vm.startPrank(user2);
mockToken.approve(address(briVault), stake);
uint256 u2Shares = briVault.deposit(stake, user2);
briVault.joinEvent(0);
vm.stopPrank();
​
// user3 (loser)
vm.startPrank(user3);
mockToken.approve(address(briVault), stake);
briVault.deposit(stake, user3);
briVault.joinEvent(5);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
​
// attacker inflates vault
address attacker = makeAddr("attacker");
mockToken.mint(attacker, 10 ether);
vm.startPrank(attacker);
mockToken.transfer(address(briVault), 10 ether);
vm.stopPrank();
​
uint256 vaultBalanceBeforeFinalize = mockToken.balanceOf(
address(briVault)
);
console.log("Inflated vault balance:", vaultBalanceBeforeFinalize);
​
// finalize winner
vm.startPrank(owner);
briVault.setWinner(0);
vm.stopPrank();
​
// winners withdraw inflated payout
uint256 u1_before = mockToken.balanceOf(user1);
uint256 u2_before = mockToken.balanceOf(user2);
​
vm.startPrank(user1);
briVault.withdraw();
vm.stopPrank();
vm.startPrank(user2);
briVault.withdraw();
vm.stopPrank();
​
console.log("User1 received:", mockToken.balanceOf(user1) - u1_before);
console.log("User2 received:", mockToken.balanceOf(user2) - u2_before);
​
assertGt(
mockToken.balanceOf(user1) - u1_before,
(1 ether * u1Shares) / (u1Shares + u2Shares)
);
}
// Logs:
// Inflated vault balance: 12955000000000000000
// User1 received: 6477500000000000000
// User2 received: 6477500000000000000

This confirms that winners were paid from an inflated vault asset pool.

Recommended Mitigation:

  1. Use tracked vault accounting instead of relying on raw ERC20 balance:

uint256 public managedAssets;
​
function deposit(...) {
managedAssets += stakeAsset;
}
​
function cancelParticipation(...) {
managedAssets -= refundAmount;
}
​
function _setFinallizedVaultBalance() internal {
finalizedVaultAsset = managedAssets;
}

  1. Use ERC4626 totalAssets() if properly implemented and not based on balanceOf.

Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
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.

Support

FAQs

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

Give us feedback!