Vana (Staking)

6.1 A rigged STREAM entity can drain other entities' principal from the shared VanaPoolTreasury

Every VanaPool entity keeps its funds in one shared VanaPoolTreasury contract, and each entity is a share based pool: activeRewardPool records the VANA it owns and totalShares records the claims against it. Entities are created by a maintainer on behalf of a third party operator, who becomes the entity owner and controls that entity's reward scheduling. A staker who exits before their bonding period matures is returned their principal and forfeits accrued rewards.

Shares are not minted in a single division. The stake(...) function of the VanaPoolStaking contract first asks the VanaPoolEntity contract for a rate, computed by the vanaToEntityShare(...) function as (totalShares * 1e18) / activeRewardPool, and then applies that rate to the deposit:

uint256 vanaToShare = vanaPoolEntity.vanaToEntityShare(entityId);
// @audit-issue The rate returned above is already floored, so the depositor
// receives floor(r) / r of the shares paid for, not a single wei less.
uint256 sharesIssued = (vanaToShare * stakeAmount) / 1e18;

Because the truncation lands on the rate rather than on the result, a depositor receives floor(r) / r of the shares they paid for. For an ordinary entity the rate is around 1e18 and the loss is invisible. When the true rate sits just below an integer m, the depositor receives only (m - 1) / m of their shares, which is as little as half at m = 2. Their wei still enters activeRewardPool, so the shortfall silently accrues to whoever already holds shares.

An entity owner can move their own entity into that regime using only owner level calls. The registration stake written by the registerEntity Stake(...) function never sets rewardEligibilityTimestamp, so it is unbonded from birth and the owner can withdraw all but a chosen dust share count. The owner then calls the distributeRewards(...) function, which is owner callable on a STREAM entity, to vest VANA into activeRewardPool without minting any shares. Leaving S0 shares against a parked amount P fixes the rate at S0 * 1e18 / P, so the owner selects m directly. This is not reachable in APY mode, where the drip computed by the calculateYield(...) function is proportional to activeRewardPool and a dust pool therefore drips dust.

The second half of the problem is that a stake made while the position is already inside its bonding period is booked at raw msg.value rather than at the value of the shares actually minted. The branch is selected by reading rewardEligibilityTimestamp as it was stored before the call, so a brand new address takes the eligible branch on its first ever deposit and is booked at value; it becomes bonded only as that same call returns. Raw wei treatment therefore applies from the second deposit onwards, and a position that is fully exited keeps its rewardEligibilityTimestamp, so the bond survives indefinitely once armed:

} else {
    // @audit-issue The cost basis grows by raw wei, while the shares bought
    // with those wei are under-issued by the floored rate.
    stakerEntity.costBasis += stakeAmount;
    // ...
}

An early exit then refunds that raw cost basis, while the entity's books are reduced by only the smaller share value. Both happen inside the _unstake(...) function:

} else {
    // @audit-issue The treasury pays out the raw cost basis.
    vanaToReturn = proportionalCostBasis;
    // ...
}

// ...

// @audit The entity gives up only shareValue, which is less than the refund.
vanaPoolEntity.updateEntityPool(entityId, shareAmount, shareValue, false);

The attacker therefore dilutes themselves inside their own entity and is then refunded pre-dilution. Because all entities share one treasury, the difference is paid out of other entities' deposits. One cycle at m = 2 against 1000e18 wei of parked capital proceeds as follows:

1. The entity is created with the 1e18 wei registration stake, minting 1e18 shares against an activeRewardPool of 1e18 wei. The rate is 1e18 and the entity behaves normally;
2. The owner unstakes all but 2000 shares. The registration position is unbonded, so this succeeds immediately and leaves 2000 shares against 2000 wei;
3. The owner calls the distributeRewards(...) function with 1000e18 wei of their own VANA and a duration of 0, and the next call to the processRewards(...) function moves it into activeRewardPool. The entity now holds 1000e18 + 2000 wei against 2000 shares, so the true rate is 1.999999999999999996, the vanaToEntityShare(...) function returns 1, and one share is worth 5e17 wei;
4. A second address controlled by the attacker makes a seed deposit of 1e18 wei. Its stored rewardEligibilityTimestamp is still 0 when the branch is chosen, so this first deposit takes the eligible branch, is booked at the value of the shares minted, and opens no gap. Its only purpose is to leave the position bonded, which the same call does before returning;
5. The same address then deposits 1414e18 wei. This time the branch test reads a rewardEligibilityTimestamp in the future, so costBasis grows by the raw 1414e18 wei to a total of 1414.5e18 wei, while the 1415 shares held are worth only about 1000.65e18 wei;
6. That address exits before the bond matures. The treasury refunds the raw 1414.5e18 wei while the entity's books give up only the 1000.65e18 wei of share value. Treasury cash is barely moved across the deposits and the refund, but the roughly 414e18 wei difference stays in activeRewardPool, backed by no cash and owned entirely by the owner's 2000 shares;
7. The owner's dust position is therefore worth about 1414.35e18 wei instead of the 1000e18 wei that was parked. When the owner finally unstakes it, the treasury pays out that full amount, and the roughly 413.85e18 wei of net gain is real cash belonging to the stakers of other entities;

The impacts are the following:

• Any honest depositor into such an entity loses up to 50% of their deposit to the entity owner's dust share position, on a single ordinary stake(...) call. The shareAmountMin parameter is the only defence and is opt-in;
• The shortfall is taken from other entities' stakers. No honest counterparty is required, since both the dust position and the depositing address belong to the attacker. Each cycle deposits b and refunds b, so treasury cash is flat while the entity's books inflate; the cash leaves only when the owner finally unstakes the dust position, and it is paid out of other entities' principal;
• The total is not bounded by the attacker's capital. Repeating the cycle walks the rate from m down to 1, and the ceiling per entity is park * (m - 1). Since m is chosen freely by how far the registration stake is drained, bounded only by S0 <= 1e18, there is no meaningful limit in code. The practical cap is the honest TVL present in the treasury;
• The treasury becomes insolvent, not merely short. Once drained, honest stakers calling unstake(...) revert with TransferFailed because the transferVana(...) function cannot pay what the books promise;
• The parked capital need not belong to the attacker. The addRewards(...) function is permissionless and the distributeRewards (...) function will schedule against an existing lockedRewardPool with msg.value of 0, so any RewardSplitter round, grant or donation already credited to the entity can be used as the park at no cost;
• The owner can guarantee exclusivity. The updateEntityStakingBlocked(...) function is owner callable with no timelock, so the pool can be kept shut to everyone and reopened only for the duration of the attacker's own deposit and exit, atomically within one transaction;
• The entity is left permanently unusable. Once activeRewardPool exceeds totalShares * 1e18 the rate floors to 0, every stake(...) reverts with InsufficientStakeAmount, and no function lowers the price per share again, so any residual lockedReward Pool is stranded;

Two configuration preconditions gate the cross entity theft, both set by a maintainer and both intended states of the system: bondingPeriod must be non-zero, since at 0 the eligible branch books costBasis = newTotalValue and the refund can never exceed the share value, and the entity must be in STREAM mode. As a concrete example, three entities holding a combined 50003e18 wei of other users' funds can be emptied by an owner who parks 1000e18 wei and rigs their own entity at m = 60: after 256 cycles and a final exit the owner nets 50003.33e18 wei, the treasury is left holding 0.67e18 wei, and an honest staker calling the unstake(...) function reverts with TransferFailed.

Recommendation

Consider issuing shares in a single division, deriving them directly from the deposit, the entity's totalShares and its activeRewardPool, so that truncation falls on the last wei of the result instead of on the conversion rate. The same reasoning applies to the share issuance inside the redelegate(...) function and to the reverse conversion inside the unstakeVana(...) function.

Independently of the rounding, consider enforcing that a refund can never exceed the value of the shares being burned, so that the treasury cannot pay out more than the entity's books give up regardless of any future rounding change. Consider also whether the cost basis recorded on a stake made during the bonding period should reflect the value of the shares actually minted rather than the raw msg.value, which would keep the two branches of stake(...) consistent with each other.

Consider giving the registration stake a floor or a bond so that totalShares cannot be reduced to dust, which also removes the precondition for this attack and prevents the permanently unusable state. The unused CannotRemoveRegistrationStake error suggests this was already intended.

Finally, consider asserting as a system wide invariant that the treasury balance is at least the sum over all entities of activeRewardPool, lockedRewardPool and accruedCommission, and covering it in the test suite. This invariant holds on every well behaved path and is violated the moment the issue is triggered.

Resolution

Fixed (commit 0d83fc5, branch feat/staking-rewards-models)

The attack combined three ingredients; the fix closes all three.

1. Single-division share issuance (the rounding half);

stake minted shares in two steps — vanaToEntityShare floors the rate ( totalShares·1e18/activeRewardPool), then multiplies by the deposit — so truncation landed on the rate, under-issuing a depositor up to (m-1)/m (50% at m=2) when the rate sat just below an integer. Fixed with a single division:

vanaToShares(entityId, vanaAmount) = vanaAmount * totalShares / activeRewardPool

used in stake, redelegate's mint, and unstakeVana's eligible branch (vanaToEntityShare kept as a price view). Truncation now falls on the last wei of the result — no rig can under-issue, and the self-brick is gone.

2. Registration seed bonded + floored (the precondition). The registration stake was unbonded and fully withdrawable, letting an owner drain their entity's totalShares to dust to skew the rate. Now registerEntityStake bonds it, and _unstake reverts CannotRemoveRegistration Stake if the entity owner would drop below minRegistrationStake. On mainnet (minReg = 0.1 VANA) rigging the rate to m=2 would need 5e16 VANA parked.

3. Refund cap (already present in a commit before the meeting). _unstake refunds min(proportionalCostBasis, shareValue), so a bonding-period exit can never pay out more than the shares are worth. This is the structural backstop: treasury and activeRewardPool move in lockstep, so no entity can pull another's cash.

Net: the cross-entity drain and treasury insolvency are unreachable. We reproduced the exact attack: pre-fix it drained 413.85 VANA and reverted an honest staker's withdrawal; with the fixes the treasury stays solvent and the drain-to-dust step reverts outright. The PoC quoted the pre-fix vanaToReturn = proportionalCostBasis, i.e. a snapshot without the cap.

Verified: TreasuryDrainAttack.t.sol (attack blocked at setup; seed floored/bonded), VanaToShares.t.sol (single-division math, incl. fuzz), and a new TreasurySolvencyInvariant.t.sol asserting your recommended system-wide invariant — treasury ≥ Σ(active + locked + accrued + stakerLocked) — held across 3.8k fuzzed operations.

Update from the client (addendum to the note above): the owner-bound floor in 0d83fc5 was found bypassable — redelegate had no floor check, and transferring entity ownership via updateEntity released it — and was replaced in 32f757f (PR #82) by a floor bound to the registrant recorded at creation, enforced on unstake, unstakeVana and redelegate-out, plus a price-per-share cap (MAX_ACTIVE_POOL_PER_SHARE = 1e12) on every mint path that protects legacy entities with no floor record. Foundry coverage: RegistrationFloorAndPriceCap.t.sol (15 tests); the solvency fuzzer now also drives redelegate, distributeRewards, ownership transfer and legacy (floor-less) entities (5700086).

6.2 A malicious entity owner can capture or withhold the rewards the splitter has already paid to their entity's stakers

The RewardSplitter contract splits a governance-funded VANA budget across VanaPool entities in proportion to the stake-seconds each entity accrued since the previous round, so an entity's share is earned by the stake its delegators committed over time. The splitter pays that share by calling the addRewards(...) function on the VanaPoolEntity contract, which only credits the entity's lockedRewardPool. For a STREAM entity, that balance reaches delegators only once a vesting schedule is installed by the distributeRewards( ...) function, which is callable by the entity owner (or a maintainer) and accepts an unconstrained duration, including 0 for an instant release.

Rewards are therefore not credited to the stakers who earned them at the moment the splitter pays; they wait under the owner's control. Whenever they do vest, the processRewards(...) function skims the owner's commission at the rate in force at that moment, and MAX_COMMISSION is 100%.

// @audit-issue The rate applied is the one current when the amount vests, which
// need not be the rate that was advertised when the amount was funded.
uint256 commission = (toDistribute * entity.commissionRate) / MAX_COMMISSION;
uint256 delegatorReward = toDistribute - commission;

entity.lockedRewardPool -= toDistribute;
entity.activeRewardPool += delegatorReward; // to delegators via share price
entity.accruedCommission += commission; // operator's cut, claimable

The updateEntityCommission(...) function settles pending rewards before applying a new rate, which is intended to make rate changes forward-only. That guard does not protect funds which have not been scheduled yet: the owner raises the rate first, so the settlement has nothing to settle and the whole balance later vests under the new rate.

1. The splitter pays the entity its share X, earned by the delegators' stake-seconds. X sits in lockedRewardPool, vesting nothing;
2. The owner calls updateEntityCommission(entityId, MAX_COMMISSION). Its settlement covers nothing, and the rate becomes 100%;
3. The owner calls distributeRewards(entityId, X, block.timestamp, 0) to install an instant entry. This function also settles before installing that entry, so again nothing vests at the old rate;
4. In any later block, the owner calls claimCommission(entityId). It settles first, so X vests in full, commission equals X, delegatorReward is 0, and X is transferred from the treasury to the owner;

Steps 2 and 3 can share a single transaction. Step 4 has to be in a later block only because processRewards(...) returns early when no time has elapsed since the last settlement.

Once that first transaction has landed, the outcome cannot be prevented, even though MAINTAINER_ROLE can call the same functions the owner used. Settlement is permissionless, so any later interaction with the entity completes the capture without the owner acting again. A maintainer lowering the rate does not help, because the updateEntityCommission(...) function settles before applying the new rate, so their own transaction vests the balance at 100% and pays the owner. Pausing does not help either: settlement is whenNotPaused, so the rate cannot be changed at all while the contract is paused, and the first settlement after unpausing still applies 100%.

The owner stakes nothing, risks nothing and serves no bonding period, since claimCommission(...) pays accruedCommission directly rather than through share value. The entire budget the splitter attributed to the delegators is taken: activeRewardPool never grows, so their share price does not move, and totalDistributedRewards records nothing as distributed. The owner can then restore the advertised rate, so the commissionRate a delegator reads afterwards is not the rate their rewards were taken at. Entities are onboarded by a maintainer, but an entity owner is otherwise an independent operator competing for delegations.

Two weaker variants of the same root cause are available even with a bounded commission rate. The owner can stake a large amount, close the entity to others with the updateEntityStakingBlocked(...) function, then release X instantly so that their snapshot position captures most of it, and unstake once the bonding period passes; an early exit still returns the principal, so the downside is roughly zero. Alternatively the owner can simply never call the distributeRewards(...) function, leaving X parked indefinitely while the entity keeps accruing stake-seconds and drawing further budget in every later round. Delegators cannot force the release, and unstaking abandons the balance, because it belongs to the entity rather than to the stakers whose stake earned it and will accrue to whoever holds shares when it is finally released. This last variant is the one a maintainer can undo, since the distributeRewards(...) function is callable by MAINTAINER_ROLE as well, so governance can schedule a parked balance without the owner's cooperation.

Recommendation

Consider revisiting the assumption that an entity owner may decide the terms on which externally funded rewards reach delegators. Rewards the splitter has already attributed to an entity's stakers should not be exposed to parameters the owner can change afterwards, and the owner should not be able to choose the moment at which those rewards land relative to their own position or their own commission rate. Consider also whether an owner should be able to leave such a balance unreleased indefinitely, and what recourse delegators or governance should have when that happens.

Resolution

Fixed in commit 2a6c350 with 3 parts:

• Dedicated, owner-proof reward track. Splitter rewards now go through addStakerRewards(entityId, payCommission, duration) ( REWARD_SPLITTER_ROLE-gated), into a separate stakerLockedRewardPool + stakerRewardSchedule kept out of lockedRewardPool. No owner function — distributeRewards, switchToStreamModel, sweep, or the APY/STREAM drip — can reach it, so the owner cannot withhold, cancel, re-time, or re-rate it. It vests linearly to delegators (via share price) over an explicit duration; overlapping rounds rebase;

• Commission fixed up front. Any owner cut is skimmed atomically at the current rate when the reward is funded; the vesting portion carries no commission, so nothing can be skimmed at a later, owner-chosen moment;

• Two-phase commission changes. Increases require proposeCommissionRate (owner) + approveCommissionRate (maintainer); decreases are immediate. An owner can no longer unilaterally spike the rate before a round;

Update from Nethermind Security

partially fixed at commit. It seems that the new REWARD_SPLITTER_ROLE is not granted inside initialize to the RewardSplitter contract. addStakerRewards will revert.

Update from the client (re: "partially fixed — REWARD_SPLITTER_ROLE is not granted"): Confirmed and fixed (93b28e7). The role cannot be granted in initialize because the splitter is deployed after the entity, so the wiring is now first-class: VanaPoolEntity.updateRewardSplitter(address) (MAINTAINER_ROLE) records the splitter (new VanaPoolEntityStorageV2.rewardSplitter), grants it REWARD_SPLITTER_ROLE and revokes the previous one, mirroring VanaPoolTreasury.updateVanaPoolEntity. A deploy script, deploy/vanaStaking/rewardSplitter-deploy.ts, deploys the splitter proxy and performs the wiring (or prints the multisig call). Tests: RewardSplitterWiring.t.sol, including "an unwired splitter cannot pay".

Update from Nethermind #2: An entity owner can frontrun the approveCommissionRate(...) call with a proposeCommissionRate(...) call to change the value and the MAINTAINER will approve an unexpected value. Consider adding a new parameter to the approveCommissionRate(...) function and take the rate as an input parameter and check that the input parameter matches the pendingCommissionRate.

Update from the client #2: Fixed in commit 8b7a3c7.

6.3 A large matured position nullifies the bonding period for subsequent deposits

The bonding period is the protocol's defensive mechanism for discouraging mercenary capital. A staker who exits before it elapses forfeits their accrued rewards into the entity's locked pool, which recycles them to the remaining stakers. When capital is added to an existing position, the contract blends the new bonding requirement with the remaining time on that position, weighted by value.

// @audit The full bonding period is divided by the combined value, so a
// large existing position shrinks the bond applied to new capital.
stakerEntity.rewardEligibilityTimestamp =
    currentTimestamp + (stakeAmount * bondingPeriod) / newTotalValue;

Because the denominator is the value of the whole combined position, the bonding period applied to the incremental capital is scaled down by whatever the staker already holds. A staker holding a matured position worth 10000 VANA who then deposits 100 VANA receives a bond of 25663 seconds, roughly seven hours, instead of the configured 30 days. The larger the existing position, the closer the effective bond is to zero, and the matured position is unaffected by the operation, so the manoeuvre can be repeated indefinitely. The cost of bootstrapping the attack is serving the bonding period once.

The redelegate(...) function reproduces the same arithmetic, weighting the incoming amount by its carried remaining bond and the destination by its own remaining time, and then assigning rather than raising the result.

uint256 weightedTime =
    (existingValue * existingRemaining + movedValue * remainingBond) / newTotalValue;
// @audit-issue The blended value is assigned, so an existing bond can be shortened.
to.rewardEligibilityTimestamp = currentTimestamp + weightedTime;

Recommendation

Consider revisiting the bonding model so that newly added value serves the necessary bonding period on its own terms. At a minimum, consider ensuring that neither the staking nor the redelegation path can ever move an existing eligibility deadline earlier than it already was.

Resolution

Acknowledged (WONTFIX): the value-weighted bond blend in stake() / redelegate() intentionally lowers deposit friction for stakers maintaining large already-bonded capital, weakens only an economic disincentive (no funds at risk, no accounting invariant broken, matured gains preserved by c583cc7), and the clean per-tranche fix costs storage/complexity or perpetually re-locks honest large stakers — so we keep it as-is.

6.4 Accrued commission can never be withdrawn from the treasury

Each entity may charge a commission, which is an operator cut taken from every reward distribution. The processRewards(...) function from the VanaPoolEntity contract takes that cut into the accruedCommission field, and the entity owner later withdraws it by calling the claimCommission(...) function, which instructs the treasury to pay out. All VANA in the system is custodied by the VanaPoolTreasury contract, and moving funds out of it requires the caller to hold DEFAULT_ADMIN_ROLE.

// @audit-issue The caller of this function is the VanaPoolEntity contract.
bool success = vanaPoolStaking.vanaPoolTreasury().transferVana(
    payable(entity.ownerAddress),
    amount
);
if (!success) {
    revert TransferFailed();
}

The treasury grants that role to two addresses when it is initialized, and the updateVanaPool(...) function only rotates it between staking contracts. The VanaPoolEntity contract is never granted the role.

function initialize(address ownerAddress, address vanaPoolAddress) external initializer {
    // ...
    _grantRole(DEFAULT_ADMIN_ROLE, ownerAddress);
    // @audit-issue Only the owner and the staking contract are granted the role.
    _grantRole(DEFAULT_ADMIN_ROLE, vanaPoolAddress);
}

Every reward distribution therefore reduces the delegators' share by the commission, while every attempt to claim that commission reverts due to the treasury's access control check.

Recommendation

Consider introducing a dedicated, narrowly scoped spending role on the VanaPoolTreasury contract that gates only the transfer functionality, and granting it to both the staking and the entity contracts as part of the upgrade procedure. Granting DEFAULT_ADMIN_ROLE to the entity contract would also resolve the revert, but it is strongly discouraged, because that role additionally authorizes upgrades and pausing of the treasury that custodies all staker principal.

Resolution

Fixed (commit 6b7b2d6)

Confirmed real: claimCommission has the entity contract call treasury.transferVana, but transferVana required DEFAULT_ADMIN_ROLE and the entity was never granted it, so every claim reverted. (On this branch the same would have broken sweepUnallocatedRewards, which also pays out through the entity.)

Fix — a narrow SPENDER_ROLE gating only transferVana:

initialize grants SPENDER_ROLE to the staking contract and DEFAULT_ADMIN only to the owner — staking no longer holds admin over the treasury. updateVanaPool rotates SPENDER_ROLE (not DEFAULT_ADMIN) between staking contracts. New updateVanaPoolEntity(address) (admin-gated) records the entity and grants it SPENDER_ROLE, rotating it off any previous entity — first-class wiring so the entity's spend right can't be silently forgotten. Backed by one appended slot (VanaPoolTreasuryStorageV2).

6.5 Redelegating into a matured position makes already earned rewards forfeitable

Stakers serve a bonding period, during which an early exit returns only the principal and forfeits the accrued rewards. Once that period has elapsed the position is matured and its rewards are withdrawable. The redelegate(...) function from the VanaPoolStaking contract lets a staker move a position between entities, and its documentation states that the full share value, principal plus accrued rewards, travels with the staker and that there is no forfeiture.

When a staker adds to an already matured position through the stake(...) function, the contract first records the matured gain into vestedRewards and resets the cost basis to the current value, so that the gain can no longer be lost before it re-bonds the position.

if (stakerEntity.rewardEligibilityTimestamp <= currentTimestamp) {
    if (stakerEntity.shares > 0) {
        uint256 oldValue = (stakerEntity.shares * shareToVana) / 1e18;
        if (oldValue > stakerEntity.costBasis) {
            stakerEntity.vestedRewards += oldValue - stakerEntity.costBasis;
        }
    }
    // @audit The cost basis absorbs the matured gain before the bond is reset.
    stakerEntity.costBasis = newTotalValue;
    // ...
}

The destination side of the redelegation performs no such record. It adds only the principal to the cost basis and then writes a new, future eligibility timestamp over the destination position.

// @audit-issue The matured gain is not crystallised before the position is re-bonded.
to.costBasis += movedCostBasis;
to.vestedRewards += movedVested;
to.shares += sharesIssued;

A staker who holds a matured position in one entity and a bonded position in another, and who moves the second into the first puts their previously safe rewards back at risk. If they exit before the newly imposed bond elapses, the unstaking logic takes the bonding branch, returns only the cost basis, and routes the difference back into the entity's locked pool for other stakers and the entity owner's commission.

Recommendation

Consider mirroring the logic that the staking path already performs, so that a matured destination position has its unrealised gain converted into cost basis before any new bonding deadline is applied to it. Consider also reviewing whether the documented promise that no forfeiture occurs during redelegation holds for every combination of source and destination maturity.

Resolution

Pushed — c583cc7. Here's the auditor summary:

[Medium] Redelegating into a matured position makes earned rewards forfeitable — Fixed (commit c583cc7)

Root cause confirmed: stake into a matured position crystallizes the earned gain into cost basis before re-bonding (costBasis = newTotalValue), so a later bonding-period exit still returns it. redelegate's destination side did not — it added only the moved principal (to.costBasis += movedCostBasis) and wrote a new future eligibility over the merged position. A staker holding a matured position in A and a bonded position in B who redelegated B→A had A re-bonded with its gain not crystallized; exiting A before the new bond took the bonding branch and forfeited that gain — breaking the documented "full value travels, no forfeiture."

Fix: on the destination, before applying the new bond, crystallize to's gain when it is matured (mirrors stake's eligible branch):

if (existingRemaining == 0 && to.shares > 0 && existingValue > to.costBasis) to.vestedRewards += existingValue - to.costBasis; to.costBasis = existingValue;

redelegate is now a proper superset of the manual path — it preserves B's rewards (which the manual path forfeits on B's bonded exit) and protects A's matured gain.

Confirmed and fixed (787e074). redelegate now crystallizes a matured source's unrealized gain into the carried cost basis and vestedRewards before the merge (the same pattern stake() applies when topping up a matured position), so a bond imposed by the destination cannot make it forfeitable; the source is debited by the proportional originals. Test test_scenario2_redelegateMaturedAintoBondedB: a 72.97 VANA matured gain moved into a bonded position and exited early is paid in full (before: forfeited).

Update from Nethermind Security

The updated logic only checks if the recipient has a matured position, but it doesn't check if from's position is matured. This means that if a matured position is redelegated, the rewards become subject to the bounding period again. Consider using a similar approach to the one used for realizing the rewards for the to's position.

6.6 Reward distribution weight can be increased by settling rewards early

A governance-funded budget is split across entities by the distribute(...) function from the RewardSplitter contract, in proportion to the stake-seconds each entity accrued since the previous round. Stake-seconds are the integral of an entity's activeRewardPool over time, and the splitter's documentation states that the design prevents a third party from skewing the split.

The quantity being integrated is also the quantity that reward settlement increases. The processRewards(...) function from the VanaPool Entity contract moves value from the locked pool into the active pool, and it carries no access control at all.

// @audit-issue This function is permissionless and raises the value that the
// stake-seconds metric integrates over time.
function processRewards(uint256 entityId) public override whenNotPaused {
    // ...
    entity.lockedRewardPool -= toDistribute;
    entity.activeRewardPool += delegatorReward;
    // ...
}

An entity that settles its pending rewards at the start of a distribution round therefore carries an elevated pool for the whole round and accrues more stake-seconds than an otherwise identical entity that settles at the end, even though both hold the same delegated stake and receive the same rewards. Anyone may trigger the settlement for any entity, so the advantage costs only gas and can be taken every round.

The effect is not marginal. A proof of concept with two entities that are identical in stake, locked rewards and timing, differing only in when their pending rewards are realised, splits a 100 ether budget 66.655562958027981345 ether to 33.344437041972018654 ether, a two-to-one division of a budget that should have been shared equally.

Recommendation

Consider weighting the distribution by delegated principal rather than by the reward-inclusive pool, for example by maintaining a separate accumulator that is updated only when stake enters or leaves an entity. Consider also reviewing the documented claim that the split cannot be skewed, since it currently addresses only attempts to reduce another entity's weight and not attempts to raise one's own.

Resolution

Fixed in d7d52ff: the cross-entity split now weights entities by committed principal (stakedPrincipal, moved only on real stake/unstake/redelegate) instead of the time-integral of activeRewardPool, and processRewards no longer checkpoints the accumulator — so vesting rewards early can no longer inflate an entity's weight (verified by a two-identical-entities test where 20 days of aggressive settlement still yields a 50/50 split; 180/180).

Confirmed and fixed (4d3fde0). A pre-upgrade entity's stakedPrincipal is now seeded from min(totalShares, activeRewardPool) rather than activeRewardPool: rewards never mint shares, so nothing settled into the pool between the upgrade and the first stake/unstake can enter the seed, and the share supply is a lower bound on principal (because a deposit of v VANA at price p ≥ 1 mints v/p ≤ v shares, the share supply is a lower bound on principal; it can only under-weight a legacy pool). A permissionless checkpointPrincipal(entityId) lets the upgrade procedure seed legacy entities immediately, closing the window itself. Tests: 400 shares against a 500 pool seed 400 (before: 500); two entities with equal share supply and 1000× different pool values seed identically.

Update from Nethermind Security

the _checkpointPrincipalSeconds(...) is only called in updateEntityPool(...) and it is supposed to only include the principal amount and not rewards. After the pool is upgraded, but before a stake(...) or unstake(...) function call happens, if a reward is distributed by the entity owner, then stakedPrincipal will be set equal to activeRewardPool which will also contain the newly distributed rewards. This can only happen in legacy pools though.

6.7 Rounding dust left by the final unstake disables an entity

Staking positions in an entity are accounted as shares. The entityShareToVana(...) and vanaToEntityShare(...) functions from the VanaPoolEntity contract convert between shares and VANA and are intended to be reciprocals of one another. Both must also address the case of an empty entity, where the next depositor is expected to bootstrap the pool at a 1:1 price.

The two functions treat this case differently. The first guards on the share supply, whereas the second guards on the size of the pool.

function entityShareToVana(uint256 entityId) external view override returns (uint256) {
    Entity storage entity = _entities[entityId];

    return entity.totalShares > 0
        ? (entity.activeRewardPool * 1e18) / entity.totalShares
        : 1e18;
}

The reciprocal branches on activeRewardPool instead, so for an entity that holds VANA but has no shares outstanding it returns 0 rather than the bootstrap price.

function vanaToEntityShare(uint256 entityId) external view override returns (uint256) {
    Entity storage entity = _entities[entityId];

    // @audit-issue This branches on the pool size rather than the share supply.
    return entity.activeRewardPool > 0
        ? (entity.totalShares * 1e18) / entity.activeRewardPool
        : 1e18;
}

This state can be reached when the last holder exits. The VANA returned to them is computed through two successive floor divisions, so whenever the share supply does not divide the pool exactly, a remainder is left behind while the share count reaches zero. With an activeRewardPool of 4e18 against a totalShares of 3e18, the final exit returns 3999999999999999999 and strands 1 wei.

From that point on the entity is unusable. Every call to the staking function computes zero shares issued and reverts, and every redelegation into the entity reverts on its slippage check, so the share supply can never become non-zero again. The activeRewardPool is decreased through paths that now revert, so the entity remains listed as active while its lockedRewardPool, including every payout the reward splitter routes to it, is unrecoverable.

Recommendation

Consider making both conversion functions agree on a single definition of the empty entity, keyed on the share supply, so that an entity holding residual VANA with no shares outstanding still quotes a bootstrap price. Additionally, consider sweeping any residual activeRewardPool back into the locked pool at the moment the share supply reaches zero, so that the dust does not accumulate against an entity with no holders.

Resolution

Update from the client: Fixed (f3c72e3 zero-share price floor; 0d83fc5 single-division vanaToShares). Both conversions key the empty entity on the share supply and quote the 1:1 bootstrap price. Dedicated test added in 770b9ae (DustBootstrap.t.sol): reproduces the stranded-dust state on a legacy entity, then asserts a stake mints 1:1, a redelegation in succeeds, and locked rewards are not stranded, with treasury solvency throughout.

Update from Nethermind Security

fixed at commit.

6.8 The executeBuyAndBurn(...) function sends tokens to a configurable sink address instead of destroying them

The RewardSplitterImplementation contract includes a buy-and-burn mechanism that sets aside a portion of the distributed tokens to be burned later. The executeBuyAndBurn() function is responsible for executing this burn by transferring the accrued native tokens. However, the function sends these funds to a configurable sink address rather than address(0).

The protocol's burn mechanism is intended to reduce the totalSupply of tokens in order to increase the price per token. Because the current logic transfers the tokens to a "sink" address instead of the zero address, the tokens are never actually destroyed. Consequently, the totalSupply does not change, meaning the feature is broken and will not achieve its desired economic goal.

function executeBuyAndBurn() external nonReentrant {
    uint256 amount = pendingBurn;
    if (amount == 0) {
        return;
    }
    address sink = buyAndBurnAddress;
    if (sink == address(0)) {
        revert InvalidAddress();
    }
    pendingBurn = 0;
    // @audit-issue Sending funds to a sink address does not reduce the token total supply.
    (bool burned, ) = sink.call{value: amount}("");
    if (!burned) {
        revert TransferFailed();
    }
    emit BuyAndBurnExecuted(sink, amount);
}

Recommendation

Consider revisiting the burn logic to ensure that the accrued tokens are sent to the zero address or properly removed from circulation.

Resolution

Fixed. sink was removed and the address(0) is used to burn.

Update from Nethermind Security

fixed at commit.

6.9 The entity registration stake never serves a bonding period

When a new entity is created, its owner must deposit a registration stake, which acts as the operator's collateral and is recorded as an ordinary staking position through the registerEntityStake(...) function from the VanaPoolStaking contract. Every other staking position is opened through the stake(...) function, which writes a share count, a cost basis and a bonding deadline.

The registration path writes only the first two fields.

// Register shares for the owner
StakerEntity storage stakerEntity = _stakers[ownerAddress].entities[entityId];
stakerEntity.shares = registrationStake;
// @audit-issue The rewardEligibilityTimestamp is never written and defaults to 0.
stakerEntity.costBasis = registrationStake;

_addStaker(ownerAddress);

Every reward in the unstaking logic compares the current time against rewardEligibilityTimestamp using a >= comparison, so the default value of 0 is read as a deadline that elapsed. The entity owner is therefore eligible for rewards on their collateral, they may withdraw it at any moment, receive the full share value including all accrued rewards, and forfeit nothing, while every ordinary staker in the same entity who deposited at the same time must serve the full bonding period or forfeit.

Collateral was intended to be locked and the evidence comes from the contract itself, which declares a dedicated error for this case, but it is never used.

// @audit-issue This error is declared but is never used anywhere in the codebase.
error CannotRemoveRegistrationStake();

Beyond the unequal treatment between an entity creator and a regular staker, removing the collateral leaves an entity that is still listed as active and eligible to receive reward splitter payouts, with no operator stake behind it. It also produces the empty entity state that the reward stream capture issue depends on.

Recommendation

Consider assigning a bonding deadline to the registration position when it is created, so the operator is subject to the same forfeiture rules as the delegators they attract. Consider additionally enforcing the already declared restriction so that the collateral cannot be withdrawn while the entity remains active.

Resolution

Fixed. Bonding period is attached to the registration stake. Also, the owner cannot unstake and leave the stake in the pool less than minRegistrationStake.

Update from the client (addendum): the seed is bonded in registerEntityStake (0d83fc5) and the non-removable floor is recorded per registrant in VanaPoolStakingStorageV3 (32f757f), so neither unstake, unstakeVana, redelegate-out nor an ownership transfer can take totalShares below it.

6.10 A one wei deposit can capture an entire elapsed reward stream

Entities may vest rewards under a stream model, in which a scheduled amount is released linearly over a time period. The _vestStream(...) function from the VanaPoolEntity contract computes how much of the active schedule has matured since its last update and advances a checkpoint so the same interval is never paid twice.

When an entity has no shares, there is nobody to receive a distribution, so the function returns early. Deliberately, and as documented in the comment, it does so without advancing the checkpoint, in order to preserve the elapsed interval for whoever stakes next.

function _vestStream(
    RewardSchedule storage schedule,
    uint256 totalShares
) internal returns (uint256 toVest) {
    // @audit-issue The checkpoint is not advanced, so the whole elapsed interval
    // is preserved and paid to whoever holds shares next.
    if (schedule.scheduledValue == 0 || totalShares == 0) {
        return 0;
    }
    // ...
}

The preserved value is not reserved for the stakers it was intended for; it is paid to whatever share supply exists at the next settlement. Because the entity is empty, the share price falls back to 1:1, so a deposit of 1 wei mints one share and makes the depositor the sole owner of the entity. Settling the rewards then moves the entire preserved amount into a pool backed by that single share.

Crucially, the guard that protects the APY model does not apply here. The staking function settles rewards before it mints shares, which for an APY entity credits any backlog to the preexisting holders. For a stream entity that settlement hits the early return above and credits nothing, so the new depositor's share is minted first and the stream vests to them afterwards.

Recommendation

Consider advancing the checkpoint when an entity has no shares outstanding, so that no vesting accrues to an empty pool, and returning the elapsed portion to the entity's residue instead. If preserving the interval is a deliberate product requirement, consider gating the preserved amount so that it can only be released to a share supply that existed while it accrued.

Resolution

Fixed (d8e459d). _vestStream now advances the watermark (and promotes ended entries) even when totalShares == 0; the elapsed portion vests to nobody and stays in lockedRewardPool as re-schedulable residue rather than being preserved for the next depositor. Reproduced your PoC: after 50 days empty, a 1 wei deposit was worth 50 ether + 1 wei; after the fix, dust, with the 50 ether retained and fully re-schedulable by the owner. Tests: StreamEmptyPoolCapture.t.sol, VestStream.t.sol.

6.11 Switching reward models leaves a stale stream schedule installed

An entity vests rewards either through a continuously compounding APY drip or through a linear stream, and a maintainer may move an entity between the two models in either direction using the updateEntityRewardModel(...) function from the VanaPoolEntity contract. The two models keep separate clocks: the APY drip advances lastUpdateTimestamp, while the stream advances its own checkpoint inside the schedule.

The dedicated function that migrates an entity into the stream model clears any previously installed schedule, and its comment explains that this is to prevent a stale entry from surviving.

processRewards(entityId);
entity.rewardModel = RewardModel.STREAM;
// @audit This sibling function clears the schedule before installing a new one.
delete entity.rewardSchedule;

The general model switch performs no equivalent clearing. It settles the outgoing model and then flips the flag, leaving the schedule and its checkpoint untouched.

processRewards(entityId);
// @audit-issue The reward schedule and its checkpoint survive the switch to APY.
entity.rewardModel = model;

While the entity sits in APY mode, the drip consumes the same lockedRewardPool that still escrows the untouched stream, and the escrow invariant is only ever checked when a new distribution is scheduled. Switching back reactivates a schedule whose checkpoint predates the entire APY phase and whose window may have elapsed in the meantime, so the next settlement releases the remaining escrow as a single lump rather than linearly.

A proof of concept installs a 100 ether stream over 100 days, an intended rate of 1 ether per day, switches to APY after one day, runs the drip for 200 days, and switches back. The next settlement releases 65.510030697081739698 ether in a single block and empties the locked pool entirely. Any shortfall between what the resurrected schedule claims and what remains in the locked pool is silently absorbed by a clamp that runs after the checkpoint has already moved, so the difference can never be vested afterwards.

Recommendation

Consider making the general model switch symmetric with the dedicated one, so that leaving the stream model clears or anchors the schedule and its checkpoint. Consider also re-asserting the escrow invariant whenever an entity reenters the stream model, so that a schedule can never be reactivated against a pool that no longer backs it.

Resolution

Fixed (d898734). updateEntityRewardModel was removed entirely; the only switch is the one-way switchToStreamModel, which deletes any existing schedule before installing a new one.

Update from Nethermind Security

fixed at commit. The function has been removed. The switchToStreamModel function should be used instead.

6.12 The VANA denominated unstake disables the minimum output check

The VanaPoolStaking contract offers two ways to withdraw. The share denominated unstake(...) function forwards a caller-supplied minimum VANA amount, which the internal logic enforces against the amount actually returned. The VANA denominated unstakeVana(...) function converts the requested amount into shares and then delegates to the same internal logic.

It passes a hard-coded zero in place of that minimum, so the guard its sibling offers is disabled on this path.

// @audit-issue The minimum VANA output is hard coded to zero on this path.
_unstake(staker, entityId, shareAmount, 0, true);

The only protection available to the caller bounds the shares burned rather than the VANA received, and it is applied after a silent clamp, so a request larger than the position is quietly reduced to the whole position instead of reverting.

if (shareAmount > stakerEntity.shares) {
    // @audit-issue An oversized request is silently reduced instead of reverting.
    shareAmount = stakerEntity.shares;
}

if (shareAmountMax > 0 && shareAmount > shareAmountMax) {
    revert InvalidSlippage();
}

A caller who passes the documented skip value for the share bound therefore executes with no protection on either side. The realistic exposure is modest, because the share price is monotonically non-decreasing and both conversions occur in the same transaction, but the asymmetry between the two withdrawal functions is unlikely to be intentional.

Recommendation

Consider offering the same value denominated protection on both withdrawal paths, so that a caller specifying an amount in VANA can bound what they receive. Consider also reverting rather than silently clamping when the requested amount exceeds the position, so that callers are not served a materially different operation from the one they requested.

Resolution

Fixed (77ab6a5), per your recommendation. unstakeVana takes a fourth parameter, vanaAmountMin, forwarded to _unstake exactly as unstake() does, and reverts with InvalidAmount on a request larger than the position instead of silently clamping. unstakeVana is not in the mainnet ABI, so the signature change affects no integrator. We deliberately did not make vanaAmount itself the implicit floor: shares are floored from the request and valued by a floored rate, so the payout lands 1–2 wei under the request on any non-1:1 price (measured 365/365 sampled requests), and an exact floor reverted essentially every call; the docstring directs callers to a floor with dust tolerance. Tests: UnstakeVanaMinOut.t.sol.

6.13 The computed bonding deadline is bounded by neither the configured period nor its maximum

The protocol advertises a single bonding period, and the updateBondingPeriod(...) function from the VanaPoolStaking contract validates that the configured value never exceeds a hard ceiling.

function updateBondingPeriod(uint256 newBondingPeriod) external onlyRole(MAINTAINER_ROLE) {
    // @audit The ceiling is validated here, on the CONFIGURED value only.
    if (newBondingPeriod > MAX_BONDING_PERIOD) {
        revert InvalidBondingPeriod();
    }
    bondingPeriod = newBondingPeriod;
    // ...
}

The deadline actually written to a position is not that value but a quotient, and nothing validates the result against either the configured period or the ceiling.

// @audit-issue Neither branch bounds the result by bondingPeriod or MAX_BONDING_PERIOD.
stakerEntity.rewardEligibilityTimestamp =
    currentTimestamp + (stakeAmount * bondingPeriod) / newTotalValue;
// ...
uint256 weightedTime =
    (oldValue * remainingTime + stakeAmount * bondingPeriod) / newTotalValue;

This produces two behaviours that the advertised period does not describe. First, newTotalValue is derived through successive floor divisions, so for a very small deposit it lands below stakeAmount and the quotient overshoots. Measured against a share price of 1.1188 and a configured period of 432000 seconds, the smallest deposit that mints a single share receives a bond of 864000 seconds, exactly 2 times the configured value; the factor is 1 + 1/n for n shares minted and has decayed to nothing by a deposit of 1e6 wei. With the period set to MAX_BONDING_PERIOD, the same deposit yields 63072000 seconds against a stated ceiling of 31536000. No economically meaningful deposit is affected, but the ceiling is not the invariant it appears to be.

Second, in the opposite direction, a top-up made part-way through a bond discards most of the time already served on the entire position, because the blend is recomputed over the combined value. With a period of 30 days, a position ten days into its bond has 1728000 seconds remaining; adding ten times the position returns the remaining time to 2513454 seconds, close to a full restart. This is the intended anti-wash behaviour rather than a defect, but it is not documented for users and it is the same formula that, in the matured-anchor case reported separately, collapses the bond toward zero instead.

Recommendation

Consider bounding the computed deadline so that a position can never be bonded for longer than the configured period, and re-asserting the ceiling on the result rather than only on the configuration. Consider also documenting that adding to a position recomputes the deadline for the whole position, since both the overshoot and the near-restart surprise users who reason about the advertised period alone.

Resolution

Fixed (0c30045). The stake bonding-branch blend and the redelegate destination blend are capped at bondingPeriod, mirroring the existing partial-unstake and redelegate-source clamps. Reproduced 28.8-day and 29.0-day deadlines written against a 7-day period after a governance reduction; both now 7 days. Upper bound only, so the value-weighted behaviour accepted under the separate Medium is unchanged. Tests: BondingDeadlineBound.t.sol.

Confirmed and fixed (602e9e8). stake()'s eligible branch now caps stakeAmount · bondingPeriod / newTotalValue at bondingPeriod, consistent with the other three deadline writes. The overshoot arises because newTotalValue is the minted shares' floored value: at a high price per share a deposit worth 1.9 shares mints one, giving a ratio of 1.9. Test test_eligibleBranchDeadlineIsCappedAtBondingPeriod: such a deposit into a 5e11 wei/share pool now receives exactly bondingPeriod (before: 1.9×).

Update from Nethermind Security

the if (stakerEntity.rewardEligibilityTimestamp <= currentTimestamp) branch doesn't bound rewardEligibilityTimestamp to the bondingPeriod. The same capping mechanism should be applied to this branch as well.

6.14 The maximum unstake view simulates only the APY reward model

The getMaxUnstakeAmount(...) function from the VanaPoolStaking contract tells an integrator how much a staker can withdraw right now. Its documentation states that it simulates reward settlement in order to quote accurate share prices, and front ends are expected to size withdrawals from the values it returns.

The simulation reproduces only one of the two reward models, and omits the operator commission entirely.

// @audit-issue The APY formula is applied unconditionally, with no check of the
// @audit-issue entity's reward model and no deduction for commission.
uint256 toDistribute =
    vanaPoolEntity.calculateYield(simulatedActiveRewardPool, entityInfo.maxAPY, timeElapsed);
if (toDistribute > entityInfo.lockedRewardPool) {
    toDistribute = entityInfo.lockedRewardPool;
}
simulatedActiveRewardPool += toDistribute;

Actual settlement branches on the entity's reward model and credits only the delegator portion of the amount released. For an entity using the stream model the view invents a drip that will never occur, while ignoring the linear vesting that will, so the quote is wrong in both directions. For any entity with a non-zero commission the simulated pool is overstated by the commission share. The structure of the data the view reads makes a correct answer impossible, since the entity information struct carries neither the reward model nor the commission rate.

There is no direct impact on funds, as the function does not modify state and the returned share count is clamped to the staker's actual holdings. The consequence falls on integrators, whose withdrawals either revert against a slippage bound or return less than quoted. The view additionally reports the treasury balance as available headroom for a single entity, when that balance is shared across all entities.

Recommendation

Consider having the view consult the entity's reward model, schedule and commission rate so that it reproduces what settlement will actually credit. A more robust alternative is to expose a preview of reward settlement on the VanaPoolEntity contract and have the view call it, so that the quoting and executing paths cannot drift apart as the reward logic evolves.

Resolution

Fixed (4197369), taking your stronger recommendation. VanaPoolEntity.previewActiveRewardPool() mirrors processRewards read-only (actual reward model, lockedRewardPool cap, commission skim, splitter track, pending queued promotion), and getMaxUnstakeAmount quotes from it. The vesting arithmetic was extracted into a single _streamDelta helper shared by the executing and quoting paths, so they cannot drift. Reproduced 100.13 vs 107.13 ether on a STREAM entity before; exact equality after, fuzzed over 1–400 days. The view also quotes the whole position exactly when it is the binding constraint. Tests: MaxUnstakePreview.t.sol.

6.15 Unclamped decrement of distributed rewards blocks early unstaking

When a staker exits before their bonding period elapses, the rewards they forfeit are returned to the entity so they can be redistributed. The returnForfeitedRewards(...) function from the VanaPoolEntity contract performs that return and also decrements the entity's cumulative totalDistributedRewards counter, since the forfeited amount is no longer considered distributed.

entity.lockedRewardPool += amount;
// @audit-issue This subtraction has no lower bound and reverts on underflow.
entity.totalDistributedRewards -= amount;

The sibling function that moves the same amount between entities during a redelegation explicitly guards the identical subtraction, and its comment states precisely why.

// @audit The author clamps the same counter here, but not in the function above.
uint256 debit = amount < from.totalDistributedRewards ? amount : from.totalDistributedRewards;
from.totalDistributedRewards -= debit;

The counter only ever accumulates the delegator portion of each distribution, while share price appreciation caused by floor rounding raises the value of existing positions without being credited to it. The difference between the sum of forfeitable gains and the counter is therefore thin, and a small residual is enough to invert the comparison. Because the minimum stake amount is not enforced, a deposit of 2 wei is sufficient to inject such a residual.

Once the counter is short, the affected staker's early exit reverts with an arithmetic panic inside the reward return, and their principal is inaccessible. The DOS is temporary rather than permanent: once the bonding period elapses, the matured branch of the unstaking logic runs, nothing is forfeited, and the reward return is never reached, so the position can always be withdrawn eventually.

Recommendation

Consider applying the same clamp that the redelegation path already uses, so that the counter returns zero instead of reverting. Consider also reviewing whether totalDistributedRewards should track rounding-induced appreciation at all, so that the two quantities being compared cannot drift apart.

Resolution

Update from the client: Fixed (f3c72e3). returnForfeitedRewards now clamps totalDistributedRewards to zero instead of underflowing, matching the redelegation path.

6.16 Redelegating can revert when the cost basis exceeds the share value

When a position is redelegated, the redelegate(...) function from the VanaPoolStaking contract transfers the reward portion of the moved value to the destination entity's accounting ledger. That reward portion is computed as the difference between the moved value and the moved principal.

// @audit-issue This subtraction is unguarded and reverts when the cost basis
// @audit-issue exceeds the current value of the moved shares.
vanaPoolEntity.redelegateDistributedRewards(
    fromEntityId,
    toEntityId,
    movedValue - movedCostBasis
);

The two operands are not derived symmetrically. Cost basis is credited at full precision when a position is opened, whereas the shares backing it are minted through two successive floor divisions. A position can therefore be worth marginally less than its recorded cost basis, and the subtraction underflows. In a worked example a zero-gain position of 5000 ether leaves a deficit of 2692 wei, which is sufficient to make the redelegation revert with an arithmetic panic.

The impact is limited. Funds are not at risk and are not locked, because the ordinary unstaking path does not perform this subtraction and remains available. The condition also resolves itself as soon as reward accrual lifts the share value above the cost basis, so the affected position is only temporarily unable to be moved.

Recommendation

Consider clamping the transferred reward portion at zero so that a position worth marginally less than its cost basis does not make redelegation revert. Consider also reviewing whether cost basis should be recorded at the value actually backed by the minted shares, so that the two quantities cannot diverge in the first place.

Resolution

Fixed (224d5a1, merged in 32f757f). The transferred reward portion is clamped at zero (movedValue > movedCostBasis ? … : 0), and bonded top-ups are booked at minted-share value so the two quantities no longer diverge. Test: test_redelegateDoesNotRevertWhenRoundingLeftPrincipalAWeiAboveValue.

6.17 The _vestStream(...) function casts the timestamp to a variable that will overflow in the future

The _vestStream(...) function from the VanaPoolEntityImplementation contract is used to compute how much a schedule has vested. The function converts the block.timestamp value to a uint32 and stores it in the schedule.lastUpdate variable. Since this is a Unix timestamp expressed in seconds, the maximum value that a uint32 can hold corresponds to approximately 136 years. Given that the Unix epoch started in 1970 and the current year is 2026, 56 years have already passed. This means the contract will behave as expected for about 80 more years until the uint32 cast silently overflows and breaks the reward vesting logic.

function _vestStream(RewardSchedule storage schedule, uint256 totalShares) internal returns (uint256 toVest) {
    // ...
    // @audit Casting block.timestamp to uint32 will overflow in approximately 80 years.
    schedule.lastUpdate = uint32(block.timestamp);
}

Recommendation

Consider updating the timestamp storage type to a larger data type to prevent potential overflow in the future.

Update from Nethermind Security

fixed at commit

6.18 The committedRewards(...) view can report a stale value before rewards are settled

An entity in the stream reward model escrows its future rewards in lockedRewardPool, and the _committedRewards(...) function reports how much of that escrow the active schedule entry and its single queued follow on entry still owe. The value is used internally to guarantee that the locked pool always backs everything the schedule promises, and it is also exposed to integrators through the committedRewards(...) view.

function committedRewards(uint256 entityId) external view override returns (uint256) {
    return _committedRewards(_entities[entityId].rewardSchedule);
}

The _committedRewards(...) function decides how much of the active entry is still outstanding purely from the current time, and it always adds the queued entry at its full nextScheduledValue.

} else if (block.timestamp >= end) {
    // @audit-issue Reports 0 as soon as the entry's end time passes, which is
    // only correct once processRewards has vested and promoted the entry.
    activeRemaining = 0; // fully vested
}
// ...
// @audit The queued entry is always added in full, even after its own vesting
// window has started.
return activeRemaining + schedule.nextScheduledValue;

These assumptions hold only right after the processRewards(...) function has settled the schedule and promoted an ended entry. When the view is read on an unsettled schedule, that is, an active entry that has ended but has not yet been promoted, or a queued entry whose vesting window has already started, the figure it returns does not match what the schedule actually still owes. The ended active entry is dropped to 0 while its funds still sit unmoved in lockedRewardPool, and the queued entry is counted at its full value even though part of it would already have vested. In practice this over reports the outstanding amount rather than under reporting it.

There is no impact on funds or on the escrow guarantee. Every state changing consumer of _committedRewards(...) calls processRewards (...) first, so the internal check that the locked pool covers the committed amount is always evaluated against a freshly settled schedule.

processRewards(entityId);
// ...
if (entity.lockedRewardPool < _committedRewards(entity.rewardSchedule)) {
    revert InsufficientRewardFunds();
}

The consequence is therefore limited to the public committedRewards(...) view: an integrator that reads it without first triggering settlement can receive a value that does not reflect the entity's true outstanding obligation and that changes the moment anyone calls processRewards(...).

Recommendation

Consider revisiting the view so that the value it reports does not depend on whether the schedule has been settled recently, or documenting that it is exact only immediately after settlement.

Resolution

Fixed (ed4f713). _committedRewards now measures the active entry's remainder at its settlement watermark (scheduledValue - vested(lastUpdate)) rather than the clock, so the view reports exactly the escrow still to leave lockedRewardPool regardless of when it is read; the queued entry continues to count in full until promoted. Immediately after settlement the value equals the previous one, so the internal escrow check is unchanged. Tests in CommittedRewardsView.t.sol, including a fuzz that committedRewards ≤ lockedRewardPool at every point, settled or not.

6.19 The configured minimum stake amount is never enforced

The VanaPoolStaking contract stores a minStakeAmount, exposes it through the staking interface, and lets a maintainer change it through the updateMinStakeAmount(...) function, which emits an event announcing the new value. Every consumer of that interface would reasonably conclude that deposits below the configured amount are rejected.

No such check exists. The value is assigned during initialization and by the setter, and is never read anywhere in the staking logic. The only size constraint applied by the stake(...) function is that the deposit must mint at least one share.

uint256 vanaToShare = vanaPoolEntity.vanaToEntityShare(entityId);
uint256 sharesIssued = (vanaToShare * stakeAmount) / 1e18;

// @audit-issue This is the only size constraint; minStakeAmount is never read.
if (sharesIssued == 0) {
    revert InsufficientStakeAmount();
}

At a share price near parity that threshold is satisfied by a deposit of one or two wei. On its own the consequence is limited to unbounded growth of the tracked staker set and of per-entity position storage, since the recipient of a deposit is an arbitrary address chosen by the caller. The more material consequence is that this dead parameter is the enabler for the reward stream capture and for the residual that triggers the unclamped decrement of distributed rewards, both reported separately.

Recommendation

Consider enforcing the configured minimum in the staking path, and reviewing whether the same floor should apply to the value moved by a redelegation. If the parameter is not intended to be enforced, consider removing it together with its setter and event, so that integrators are not misled into relying on a constraint the protocol does not apply.

Resolution

Fixed (7752e34). minStakeAmount is now enforced on exit paths as well: a partial unstake/unstakeVana must leave nothing or a position worth at least the minimum, and redelegate must move at least the minimum and leave nothing or at least the minimum in the source. Full exits remain unrestricted. Registration floors exceed the minimum on both chains, so registrants are never trapped between the two rules. Tests in MinStakeResidual.t.sol.

Update from Nethermind#3 (Telegram): Now in legacy pools, if someone have a position which is worth less than minShareAmount, they won't be able to delegate their position and will need to unstake in order to stake in different pool. It can be intended design choice though.

Agreed and refined (7171d8b). A whole-position redelegation is now always allowed, so a legacy position below the minimum can still migrate (relocating it creates no dust); only splitting a position must respect minStakeAmount on both the moved and the residual side. Full exits remain unrestricted. Test test_fullRedelegateOfASubMinimumLegacyPositionIsAllowed.

Update from Nethermind Security

fixed at commit.

Update from Nethermind#2: the minStakeAmount is not enforced on the unstake && redelegate paths. A user can unstake or redelegate such that they will end up with a position that's below minStakeAmount.

6.20 The max amount quoted by getMaxUnstakeAmount(...) can be rejected by unstakeVana(...) after the minimum-stake-on-exit rule

The getMaxUnstakeAmount(...) view is what an interface calls to populate a "max" button: it returns both maxVana (the most VANA a staker can take out) and maxShares (the same ceiling expressed in shares). For a reward eligible position it quotes the full value of the staker's shares.

uint256 userShareValue = (stakerEntity.shares * shareToVana) / 1e18;
// ...
} else {
    // @audit Reward eligible: the quote is the full value of ALL the shares.
    userWithdrawable = userShareValue;
}

A caller that then passes maxVana to unstakeVana(...) has the amount converted back to a share count with vanaToShares(...), which floors. Because maxVana was itself produced by a flooring multiplication, the round trip lands a few share units below the staker's full holding, so the burn leaves a tiny remainder rather than clearing the position.

shareAmount = vanaPoolEntity.vanaToShares(entityId, vanaAmount);

The minimum-stake-on-exit rule added in the re-review then rejects that remainder, because it is worth far less than minStakeAmount.

uint256 remainingShares = stakerEntity.shares - shareAmount;
// @audit-issue The dust left by the maxVana round trip is below minStakeAmount,
// so the call quoted as the maximum reverts.
if (remainingShares > 0 && (remainingShares * shareToVana) / 1e18 < minStakeAmount) {
    revert InsufficientStakeAmount();
}

The result is a mismatch between what the view promises and what the withdrawal accepts: for a matured position whose share price is not a whole number (which is any position that has accrued rewards), calling unstakeVana(...) with the quoted maxVana reverts with InsufficientStakeAmount. There is no impact on funds, and the staker can still exit in full by using the share denominated path with the quoted maxShares, or by lowering the requested amount so the remainder clears the minimum. The consequence is limited to the honesty of the maxVana figure on the VANA denominated withdrawal path: an integrator that wires the "max" button to unstakeVana(maxVana) will see it revert. This is the same class of view versus execution mismatch as the original maximum-unstake finding, reintroduced on the VANA path by the new exit rule.

Recommendation

Consider making the VANA denominated maximum consistent with what the withdrawal will accept, so that the quoted value either clears the position entirely or leaves at least the minimum, or documenting that the share denominated maxShares is the value to use for a full exit.

Resolution

unstakeVana now serves a request that would strand a sub-minimum remainder as the full exit it amounts to, provided shareAmountMax admits the whole position (the maxShares returned alongside maxVana; 0 is not consent) and the registration floor allows it, so unstakeVana(maxVana, maxShares, …) clears a matured position even against a slightly stale quote, while any other request keeps the existing exit rule and nothing is burned beyond the caller's bound.

6.21 Unstaking cost grows linearly with the number of entities ever created

The VanaPoolStaking contract keeps an active and an inactive staker list. Adding a staker to the active list is a constant-time operation performed whenever a position is opened. Removing one, which happens on every single unstake, first determines whether the staker still holds a position anywhere, by scanning every entity identifier that has ever been issued.

function _removeStaker(address staker) internal {
    uint256 entitiesCount = vanaPoolEntity.entitiesCount();
    bool hasStake = false;
    // @audit-issue This scans every entity ever created, on every unstake.
    for (uint256 i = 1; i <= entitiesCount; i++) {
        if (_stakers[staker].entities[i].shares > 0) {
            hasStake = true;
            break;
        }
    }
    // ...
}

Each iteration reads a distinct cold storage slot, and the loop bound only ever increases, because the entity counter is never decremented and entity removal is commented out. The scan also runs on partial withdrawals where the position clearly survives, and the early exit only helps stakers whose lowest-numbered position is found quickly, so a staker whose only position is a recently created entity always pays the full scan.

The result is that the cost of leaving the protocol grows without bound as the protocol grows, and can never be reduced, while the cost of entering remains constant. Entity creation is restricted to a maintainer, so the counts at which withdrawal would become genuinely impossible are not attacker-reachable; the realistic impact is a permanent and steadily increasing burden on the one operation that must always remain available to users.

Recommendation

Consider tracking each staker's non-empty positions directly, for example with a set that is updated as positions are opened and closed, so that list membership can be determined without scanning the entire entity space. Consider also avoiding the check entirely on partial withdrawals where the position demonstrably survives.

Resolution

Partially addressed (2b5849b). _removeStaker's scan now runs only on a full exit; a partial withdrawal leaves the position alive, so the staker is known to be active and the scan is skipped (behaviour unchanged, pinned by test_partialUnstakeKeepsStakerActive_fullExitDeactivates). Replacing the full-exit scan with a per-staker position count needs a storage slot and a migration for existing stakers; acknowledged and left as a follow-up.

6.22 The receive() function doesn't check if msg.value = 0

The receive() function from the RewardSplitterImplementation contract is used to fund the splitter's distributable balance. However, it does not check if the sent msg.value is zero. This allows anyone to call the function without attaching any value and spam the contract to emit worthless Funded events.

/// @notice Fund the splitter's distributable balance.
receive() external payable {
    // @audit no check for msg.value == 0
    emit Funded(msg.sender, msg.value);
}

Recommendation

Consider emitting the event only if msg.value is > zero.

Resolution

Fixed (2b5849b). receive() emits Funded only when msg.value > 0; a zero-value call is still accepted but emits nothing. Test: test_zeroValueFundingEmitsNoEvent (RewardSplitter.t.sol).

6.23 The updateEntityPool(...) function contains a redundant role check

The updateEntityPool(...) function from the VanaPoolEntityImplementation contract is used to update the entity stake information. The function uses the onlyRole(VANA_POOL_ROLE) modifier to restrict access, but the function's body contains the same check hasRole(VANA_POOL_ROLE, msg.sender) that reverts with NotAuthorized(). Because the modifier already enforces this requirement, the internal check is redundant.

function updateEntityPool(
    uint256 entityId,
    uint256 shares,
    uint256 amount,
    bool isStake
) external override whenNotPaused onlyRole(VANA_POOL_ROLE) {
    // @audit This check is redundant because of the onlyRole modifier.
    if (!hasRole(VANA_POOL_ROLE, msg.sender)) {
        revert NotAuthorized();
    }
    // ...
}

Recommendation

Consider removing the redundant role check from the function body.

Resolution

Fixed (2b5849b). The in-body hasRole check was removed; onlyRole(VANA_POOL_ROLE) alone gates the function. Test: test_updateEntityPoolStillGatedWithoutTheRedundantCheck.

6.24 The codebase contains unused error and event declarations

The VanaPoolEntityImplementation and VanaPoolStakingImplementation contracts declare several errors and events that are never used.

// vanaPoolEntity/VanaPoolEntityImplementation.sol
error InvalidEntityId();
error EntityNameAlreadyExists();
error NameTooShort();
error StakersStillPresent();

// vanaPoolStaking/VanaPoolStakingImplementation.sol
event EntityStakeRegistered(uint256 indexed entityId, address indexed ownerAddress);
error InsufficientShares();
error EntityNotFound();
error NotEntityOwner();
error CannotRemoveRegistrationStake();
error NotAuthorized();

Recommendation

Consider removing the unused error and event declarations from the contracts.

Resolution

Fixed (2b5849b, c2b250c). Removed: VanaPoolEntity InvalidEntityId, EntityNameAlreadyExists, NameTooShort, StakersStillPresent, NotAuthorized (its only use was the redundant check below); VanaPoolStaking event EntityStakeRegistered, InsufficientShares, EntityNotFound, NotEntityOwner, NotAuthorized. Two names on your list are now in use and were kept: CannotRemoveRegistrationStake (raised by the registration floor) and VanaPoolEntity's NotEntityOwner.

6.25 The calculateYield(...) interface declares its first two parameters in the wrong order

The VanaPoolEntity contract exposes a pure helper that integrators can call to reproduce the protocol's yield calculation. The interface and the implementation disagree about what the first two arguments mean.

// @audit-issue The interface declares apy first and principal second.
function calculateYield(
    uint256 apy,
    uint256 principal,
    uint256 time
) external pure returns (uint256);

The implementation takes them in the opposite order, as principal then apy. All three parameters are uint256, so the selector is identical, the override compiles silently, and the single in-repository caller passes them positionally in the implementation's order and is therefore correct. Any integrator coding against the published interface will transpose the two values and receive a meaningless result, with no revert to signal it.

Recommendation

Consider renaming the interface parameters so that they match the implementation, and reviewing the other interface declarations for the same kind of drift.

Resolution

Fixed (2b5849b). Interface renamed to (principal, apy, time) to match the implementation. Per your recommendation we scanned all three interfaces for the same drift and aligned the two other mismatches (IVanaPoolStaking.unstake's share parameter was named amount; IVanaPoolEntity.updateVanaPool's parameter name).

6.26 The buy and burn sink can be cleared while a reserve is still outstanding

The RewardSplitter contract sets aside a configurable fraction of each distributed budget in pendingBurn, which is later flushed to the configured sink. The updateBuyAndBurn(...) function requires a non-zero sink only when the rate itself is non-zero.

// @audit-issue The sink may be set to the zero address whenever the rate is
// @audit-issue zero, including while pendingBurn is still non-zero.
if (newBurnRate > 0 && newBuyAndBurnAddress == address(0)) {
    revert InvalidAddress();
}

Disabling the feature by passing a zero rate and a zero sink is therefore accepted even when a reserve has already accrued. In that state the flush reverts on its own zero address check, while both the distribution and the withdrawal paths subtract pendingBurn from the balance they are allowed to touch, so the reserve is simultaneously unspendable and unwithdrawable. It is recoverable by re-configuring a sink, so this is a configuration trap rather than a loss.

Recommendation

Consider requiring a non-zero sink whenever a reserve is outstanding, regardless of the rate being set, so that the feature cannot be switched off in a way that strands funds already set aside.

Resolution

Removed the configurable sink entirely instead of guarding it. The cut now burns directly to address(0):

buyAndBurnAddress and updateBuyAndBurn(rate, addr) (with the flagged zero-address check) removed. Added BURN_ADDRESS = address(0); updateBurnRate(rate) sets only the rate; executeBurn() sends pendingBurn to BURN_ADDRESS unconditionally (permissionless). The trap is gone: no sink to misconfigure, and pendingBurn is always flushable via executeBurn() regardless of rate — so it can't be stranded.