6.1 Fee calculations round down instead of rounding up in favor of the protocol
The protocol implements two types of fees: exit fees charged when users request to unlock their shares, and management fees that accrue continuously as virtual shares. Both fee calculations use integer division which rounds down toward zero, causing the protocol to collect less fees than intended, especially for small amounts.
The exit fee calculation in requestUnlock(...) multiplies the user's shares by the exit fee basis points and divides by BIPS, using integer division that rounds down:
// @audit - should round in favor of the protocol
sharesToFee = uint96(uint256(shares) * exitFeeInBips / BIPS);
Similarly, the management fee calculation in _updateFees() uses integer division for both the fee percentage and time-weighted calculations:
uint256 feeShares = (ERC20Upgradeable.totalSupply() +
_managementFee.virtualSharesSnapshot) * _managementFee.bips / BIPS;
uint256 feeSharesTimeWeighted = feeShares * time / YEAR;
// @audit - Both divisions round down, losing fees for the protocol
_managementFee.virtualSharesSnapshot = uint96(_managementFee.virtualSharesSnapshot
+ feeSharesTimeWeighted);
When dealing with small amounts, integer division can result in fees rounding down to zero, meaning the protocol receives no fee at all for those transactions. For example, with an exit fee of 5 basis points (0.05%), a user unlocking 1999 wei worth of shares would result in 1999 * 5 / 10000 = 0 wei fee after integer division.
Recommendation
Consider implementing rounding-up logic for fee calculations to ensure the protocol always collects fees in its favor. This is a common best practice in DeFi protocols to prevent gradual fee losses from rounding down.
Resolution
We changed to a round-up approach when calculating protocol fees to round in the protocol's favor. When all fees are set to zero, no rounding takes place and the protocol will still accumulate zero fees as before.
We added these changes and associated tests in commit 31b9c1f0aafddf5c17106807162241d63bfe4474.
6.2 Phase 2 logic in bonding and unbonding functions is unnecessary and adds complexity
The _doBonding(...) and _doUnbonding(...) functions implement a two-phase distribution mechanism where Phase 1 handles rebalancing towards optimal allocations and Phase 2 distributes any remaining funds proportionally. Phase 1 calculates and fills under-allocations (bonding) or drains over-allocations (unbonding) based on the imbalance between current stake distribution and target distribution derived from node weights. Any funds remaining after Phase 1 would be distributed in Phase 2 proportionally by weight or remaining stake.
However, Phase 2 can only contain at most (number_of_nodes - 1) wei because underAllocation is calculated using newTotalStaked, which already includes the requestedBonding amount ( newTotalStaked = totalPooled - batchWithdrawals). Since optimal allocations are computed based on this future total that accounts for the new funds, underAllocation will always be approximately equal to requestedBonding. This means Phase 1 distributes essentially all of requestedBonding to fill the deficits, leaving only negligible rounding dust (at most (number_of_nodes - 1) wei from integer division) for Phase 2.
// Amount to distribute to under-allocated nodes
uint256 phase1 = requestedBonding < underAllocation ? requestedBonding : underAllocation;
// Remaining amount to distribute equitably to all nodes
uint256 phase2 = requestedBonding - phase1;
// ... Phase 1 distribution ...
// @audit-issue Phase 2 distribution handles only dust amounts that round to zero
uint256 phase2Amount = phase2 > 0 ? phase2 * _nodes[i].weight / _totalWeight : 0;
The dust created from Phase 2 (and Phase 1 rounding) is already handled by the batch submission mechanism in submitBatch(...), which calculates the difference between requested amounts and actual amounts bonded/unbonded, then carries forward any dust to the next batch:
if (batchDepositRequest.assets > batchWithdrawRequest.assets) {
uint96 bondedAmountEvm = _doBonding(...);
uint96 dust = batchDepositRequest.assets - batchWithdrawRequest.assets
- bondedAmountEvm;
// @audit Dust from Phase 2 (and Phase 1 rounding) is handled here
if (dust > 0) {
batchDepositRequests[_currentBatchId + 1].assets = dust;
}
}
This means the Phase 2 logic adds unnecessary complexity to the codebase without providing meaningful functionality, as any funds that would go through Phase 2 simply become dust that gets processed in the next batch submission anyway.
Recommendation
Consider removing the Phase 2 distribution logic from both _doBonding(...) and _doUnbonding(...) functions, as it only handles negligible dust amounts that are already properly managed by the existing batch dust handling mechanism. The Phase 1 logic is sufficient to handle all meaningful rebalancing operations, and any rounding losses will naturally become dust that gets carried forward to subsequent batches where they can accumulate to meaningful amounts before being processed.
Resolution
After mimicking the logical flow in excel sheets, we confirmed that phase2 is indeed only handling dust. We simplified the algo in both _doBonding() and _doUnbonding() to use a single phase approach. We also refactored the helper function to better explain that it is doing the heavy lifting of pro rata weight based syncing.
Addressed in commit e99d4511bdb771a76f88b8182b9e5e9844e30889.
6.3 The updateWeights(...) function reverts with an incorrect error message
The updateWeights(...) function in the Registry contract is used to modify the stake allocation weights for nodes. The logic correctly prevents increasing the weight of a node that has already been disabled.
However, when this check fails (i.e., an attempt is made to increase the weight of a disabled node), the function reverts with the ActiveNode() error. This error message is misleading, as it implies the node is active when the check is specifically for disabled nodes. This can cause confusion for users or off-chain tooling monitoring for failed transactions, as the revert reason does not match the actual condition that failed.
function updateWeights(WeightDelta[] calldata weightDeltas) external {
// ...
uint256 len = weightDeltas.length;
for (uint256 i; i < len; ++i) {
if (weightDeltas[i].isIncreasing) {
Node storage node = getNodeByNodeId(weightDeltas[i].nodeId);
// @audit-issue This check reverts with ActiveNode() error when
// the node is disabled.
if (isNodeDisabled[node.id]) revert ActiveNode();
// ...
} else {
// ...
}
}
// ...
}
Recommendation
Consider changing the error thrown in this scenario to one that more accurately reflects the reason for the revert, such as NodeDisabled(). This will improve the clarity of the function's behavior when it reverts.
Resolution
We see how this could be confusing.
We changed the error message in commit af0a8346cc7bfaff1ae65aa02f0701f4ba9dff75.
