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

MEDIUM-05: _setFinallizedVaultBalance Lacks Protection Against Resets

mostafapahlevani93

Root + Impact

The _setFinallizedVaultBalance() function can theoretically be called multiple times, recalculating the vault balance and potentially allowing manipulation of the critical finalizedVaultAsset value.

Description

Normal behavior expects finalizedVaultAsset to be immutably set when the winner is determined, capturing the exact vault balance at that moment for fair distribution.

The current implementation recalculates from balanceOf() every time without checking if already set. While currently only called from setWinner() which has a guard, the function itself is not defensive.

function _setFinallizedVaultBalance() internal returns (uint256) {
if (block.timestamp <= eventStartDate) {
revert eventNotStarted();
}
​
// @> No check if finalizedVaultAsset is already set
// @> Recalculates every time, potentially changing value
return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));
}

Risk

Likelihood:

  • Currently protected by _setWinner flag in parent function

  • However, future refactoring could expose this function

  • Internal functions should be self-defensive

  • Owner could call this from an upgrade or new function

Impact:

  • If vault receives additional tokens after setWinner() (accidental transfers, rewards, etc.)

  • Second call to _setFinallizedVaultBalance() increases finalizedVaultAsset

  • Winners receive more than they should

  • Contract becomes insolvent if balance later decreases

  • Conversely, if balance decreases (somehow), winners receive less

  • Breaks the fairness guarantee of locked-in distribution

Proof of Concept

// Winner is set at time T1
vault.setWinner(0);
// finalizedVaultAsset = 100,000 tokens
// totalWinnerShares = 10,000
​
// Someone accidentally sends 50,000 tokens to vault
token.transfer(address(vault), 50000e18);
​
// Hypothetically, if _setFinallizedVaultBalance is called again
// (through upgrade, bug, or new function)
vault._setFinallizedVaultBalance(); // If made public
// finalizedVaultAsset = 150,000 tokens (increased!)
​
// Winners now withdraw based on 150,000 instead of 100,000
// Each winner gets 50% more than deserved
// Includes accidentally sent funds in distribution

Recommended Mitigation

function _setFinallizedVaultBalance() internal returns (uint256) {
if (block.timestamp <= eventStartDate) {
revert eventNotStarted();
}
​
+ // Prevent resetting the finalized balance
+ if (finalizedVaultAsset != 0) {
+ revert("Vault balance already finalized");
+ }
​
return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));
}

Alternative: Make it truly immutable

+bool public isBalanceFinalized;
​
function _setFinallizedVaultBalance() internal returns (uint256) {
if (block.timestamp <= eventStartDate) {
revert eventNotStarted();
}
​
+ require(!isBalanceFinalized, "Vault balance already finalized");
+ isBalanceFinalized = true;
​
return finalizedVaultAsset = IERC20(asset()).balanceOf(address(this));
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
mostafapahlevani93 Submitter
18 days ago
bube Lead Judge
15 days ago
bube Lead Judge 14 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Inflation attack

Support

FAQs

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

Give us feedback!