Findings
High Risk
3 findings
3 fixed
0 acknowledged
Medium Risk
3 findings
3 fixed
0 acknowledged
Low Risk
2 findings
2 fixed
0 acknowledged
Informational
3 findings
2 fixed
1 acknowledged
High Risk3 findings
The amounts for bonding and unbonding in submitBatch() are incorrect
Severity
- Severity: High
≈
Likelihood: High×
Impact: Medium Submitted by
cccz
Description
submitBatch()
will bond or unbond Hype from nodes based on the requested deposits and withdrawals for each batch.if (batchDepositRequest.assets > batchWithdrawRequest.assets) { // Net ingress uint96 bondedAmountEvm = _doBonding(batchDepositRequest.assets - batchWithdrawRequest.assets, _totalPooled); uint96 dust = batchDepositRequest.assets - bondedAmountEvm; if (dust > 0) { batchDepositRequests[_currentBatchId + 1].assets = dust; } batchSubmission.unbondingThaw = _safeIncrease(batchSubmission.unbondingThaw, _cooldowns.unbondingFreezeFromBonding); } else if (batchWithdrawRequest.assets > batchDepositRequest.assets) { // Net egress if (block.timestamp <= batchSubmission.unbondingThaw) revert UnbondingFreeze(); uint96 unbondedAmountEvm = _doUnbonding(batchWithdrawRequest.assets - batchDepositRequest.assets, _totalPooled);
The problem here is that it should bond or unbond the difference between deposits and withdrawals, rather than the whole deposit or the whole withdrawal.
The impact is as follows:
- Bonding too much amount, resulting in users being unable to withdraw funds.
- Unbonding too much amount, resulting in funds being stuck in the Vault and not getting staked by nodes.
Recommendation
Consider using differences to bond or unbond.
if (batchDepositRequest.assets > batchWithdrawRequest.assets) { // Net ingress- uint96 bondedAmountEvm = _doBonding(batchDepositRequest.assets, _totalPooled);+ uint96 bondedAmountEvm = _doBonding(batchDepositRequest.assets - batchWithdrawRequest.assets, _totalPooled); uint96 dust = batchDepositRequest.assets - bondedAmountEvm; if (dust > 0) { batchDepositRequests[_currentBatchId + 1].assets = dust; } batchSubmission.unbondingThaw = _safeIncrease(batchSubmission.unbondingThaw, _cooldowns.unbondingFreezeFromBonding); } else if (batchWithdrawRequest.assets > batchDepositRequest.assets) { // Net egress if (block.timestamp <= batchSubmission.unbondingThaw) revert UnbondingFreeze();- uint96 unbondedAmountEvm = _doUnbonding(batchWithdrawRequest.assets, _totalPooled);+ uint96 unbondedAmountEvm = _doUnbonding(batchWithdrawRequest.assets - batchDepositRequest.assets, _totalPooled);
requestUnlock() does not charge exitFee to the caller
Severity
- Severity: High
≈
Likelihood: High×
Impact: Medium Submitted by
cccz
Description
When a user calls
requestUnlock()
to request a withdrawal, no exitFee is charged to that user.function requestUnlock(uint96 shares) external whenNotPaused { ExitFee memory _exitFee = exitFee; // shadow (SLOAD 1 slot) uint96 sharesToToll = uint96(uint256(shares) * _exitFee.bips / BIPS); uint96 sharesToUser = uint96(uint256(shares) - sharesToToll); if (sharesToUser == 0) revert NoChange(); _updateFees(); uint96 spotValue = convertToAssets(sharesToUser); if (spotValue == 0) revert MinimumUnlock(); uint40 _currentBatchId = currentBatchId; // shadow // Update user's unlock requests userUnlockRequests[msg.sender].push(UnlockRequest({ shares: sharesToUser, spotValue: spotValue, batchId: _currentBatchId })); // Update current batch Batch memory _batchWithdrawRequest = batchWithdrawRequests[_currentBatchId]; // shadow (SLOAD 1 slot) _batchWithdrawRequest.assets += spotValue; _batchWithdrawRequest.shares += sharesToUser; batchWithdrawRequests[_currentBatchId] = _batchWithdrawRequest; // Escrow toll if (sharesToToll > 0) { _exitFee.escrowShares += sharesToToll; exitFee = _exitFee; } // Escrow shares token.transferFrom(msg.sender, address(this), sharesToUser);
Instead, the exitFee is cached to
escrowShares
and then charged together with the management fee.function mintProtocolShares(address to) external whenNotPaused onlyRole(ROLE_FEE_CLAIMER) { _updateFees(); uint96 shares = managementFee.virtualSharesSnapshot + exitFee.escrowShares; if (shares == 0) revert NoChange(); managementFee.virtualSharesSnapshot = 0; exitFee.escrowShares = 0; token.mint(to, shares);
This result in the exitFee not being charged to the user who requested the unstake, but the remaining users.
Malicious users can repeatedly request unstake and then cancel unstake, thereby making other users suffer losses.
Recommendation
It is recommended to charge users who request to unstake, and to no longer mint
escrowShares
, but to sendescrowShares
to the fee recipient.Dust is incorrectly calculated
Severity
- Severity: High
≈
Likelihood: Medium×
Impact: High Submitted by
HickupHH3
Description
The dust calculation uses the full batch deposit / withdrawal asset amounts, but it should using the net amounts instead.
Proof of Concept (if required)
function test_depositDustAmount() public setRoles setNodesAndWeights { vm.startPrank(user); vault.deposit{value: 1 ether}(user); vault.requestUnlock(1 ether - 1e10 + 1); // such that amountToBond is 0 vault.submitBatch(); // check dust for current batch, it's 1 ether when it should be 1e10 - 1 (uint96 assets,) = vault.batchDepositRequests(1); assertEq(assets, 1 ether); }
Recommendation
- uint96 dust = batchDepositRequest.assets - bondedAmountEvm;+ uint96 dust = batchDepositRequest.assets - batchWithdrawalRequest.assets - bondedAmountEvm;
likewise for the other side:
- uint96 dust = batchWithdrawRequest.assets - bondedAmountEvm;+ uint96 dust = batchWithdrawRequest.assets - batchDepositRequest.assets - bondedAmountEvm;
Medium Risk3 findings
Bonding and unbonding using incorrect argument to call _getImbalances().
Severity
- Severity: Medium
≈
Likelihood: High×
Impact: Medium Submitted by
cccz
Description
_getImbalances()
will calculate the imbalance based on the new total stake.function _getImbalances(Node[] memory nodes, uint96 newTotalPooled) private view returns ( uint96 overAllocation, uint96 underAllocation, uint96[] memory overAllocations, uint96[] memory underAllocations ) { uint256 _totalWeight = Registry.totalWeight; // shadow uint256 len = nodes.length; overAllocations = new uint96[](len); underAllocations = new uint96[](len); for (uint256 i; i < len; ++i) { uint256 stakedAmountCurrent = nodes[i].staked; uint256 stakedAmountOptimal = _totalWeight > 0 ? uint256(nodes[i].weight) * uint256(newTotalPooled) / _totalWeight : 0;
But
_doBonding()
and_doUnbonding()
used incorrect arguments to call it.Below
totalPooled
represents the total assets deposited by the user, andtotalStaked
represents the assets already staked to nodes.And
totalPooled == totalStaked + stakingReserveCore + batchDepositRequest.asset
.-
For
_doBonding()
, considertotalStaked == 10000
,stakingReserveCore == 0
,batchDepositRequest.assets == 2000
,batchWithdrawRequest.assets == 0
, andtotalPooled == 12000
.When
_doBonding()
calls_getImbalances()
, it should pass10000 + 2000
instead of12000 + 2000
.(, uint96 underAllocation,, uint96[] memory underAllocations) = _getImbalances(_nodes, _totalPooled + amountToBond);
-
For
_doUnbonding()
, considertotalStaked == 10000
,stakingReserveCore == 0
,batchDepositRequest.assets == 2000
,batchWithdrawRequest.assets == 3000
, andtotalPooled == 12000
.After fixing Finding#1, when
_doUnbonding()
calls_getImbalances()
, it should pass10000 - 1000
instead of12000 - 1000
.(uint96 overAllocation,, uint96[] memory overAllocations,) = _getImbalances(_nodes, _totalPooled - amount);
Recommendation
When calling
_getImbalances()
on bonding and unbonding, replace_totalPooled
in the arguments withtotalStaked
.syncStaking() can be exploited for various purposes
Severity
- Severity: Medium
Submitted by
HickupHH3
Description
Because this function is permissionless and can be called by anyone,
-
if there are pending rewards that haven't been synced, it would be advantageous to call this after
deposit()
to dilute the rewards, or beforerequestUnlock()
to ensure their shares value increased. -
Crucially, write operations via the
CoreWriter
are not atomic, so there will be a delay between bonding / unbonding delegations in_doBonding()
/_doUnbonding()
and its updated state being reflected on the node. As such, it is rather simple to write a contract that calls bothsyncStaking()
andsubmitBatch()
in a single transaction, causing the delegation deltas to be recognised as rewards / slashes instead.
Proof of Concept
function test_syncAfterDepositBeforeWithdraw() public setRoles setNodesAndWeights { vm.startPrank(user); vault.deposit{value: 1 ether}(user); vault.submitBatch(); // appreciate share price writer.addStakingReward{value: 0.01 ether}(address(vault)); // may be advantageous to sync after calling deposit uint96 sharesReceived = vault.deposit{value: 1 ether}(user); uint96 sharesValue = vault.convertToAssets(sharesReceived); vault.syncStaking(); uint96 newSharesValue = vault.convertToAssets(sharesReceived); assertGt(newSharesValue, sharesValue); // likewise, for withdraw, may be advantageous to sync before calling withdraw writer.addStakingReward{value: 0.01 ether}(address(vault)); uint96 sharesToWithdraw = 0.1 ether; sharesValue = vault.convertToAssets(sharesToWithdraw); vault.syncStaking(); newSharesValue = vault.convertToAssets(sharesToWithdraw); assertGt(newSharesValue, sharesValue);}
Recommendation
Consider making
syncStaking()
permissioned.Vault is susceptible to share inflation attack
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: High Submitted by
HickupHH3
Description
The vault contains a common vulnerability affecting vault deposits whereby the first depositor can massively inflate the share price to cause subsequent depositors to receive zero/fewer shares than expected, thereby draining their deposits.
Proof of Concept
function test_firstDepositorInflationAttack() public setRoles setNodesAndWeights { address attacker = makeAddr("attacker"); vm.deal(attacker, 100 ether); vm.startPrank(attacker); vault.deposit{value: 0.01 ether}(attacker); sHype.approve(address(vault), type(uint256).max); vault.requestUnlock(0.01 ether - 1); vault.submitBatch(); // attacker deposits a large amount to inflate sHype share price vault.contributeToPool{value: 90 ether}(attacker); // user deposits 1 ether, gets 0 shares vm.startPrank(user); uint96 actualShares = vault.deposit{value: 1 ether}(user); assertEq(actualShares, 0); }
Recommendation
A common mitigation is to burn a small amount of the shares minted by the first depositor.
Low Risk2 findings
Unbonding should withdraw from stakingReserveCore
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: Medium Submitted by
cccz
Description
The assets deposited by users consist of three parts:
batchDepositRequests.assets
,stakingReserveCore
, andtotalStaked
.When processing user withdrawal requests,
batchDepositRequests.assets
is first used to cover the request, followed by unbonding.function _doUnbonding(uint96 amount, uint96 _totalPooled) private returns (uint96 unbondSummationEvm) { Node[] memory _nodes = Registry.nodes; // shadow (SLOAD 2n-nodes slots) uint256 n = _nodes.length; (uint96 overAllocation,, uint96[] memory overAllocations,) = _getImbalances(_nodes, _totalPooled - amount); // Amount to withdraw from over-allocated agents uint256 phase1 = amount < overAllocation ? amount : overAllocation; // Remaining amount to withdraw equitably from all agents uint256 phase2 = amount - phase1;
The problem here is that during unbonding, only
totalStaked
is considered, notstakingReserveCore
. In the bad case,totalStaked
may not be sufficient to cover the remaining withdrawal requests, leading to unbonding failure.Recommendation
Consider withdrawing from
stakingReserveCore
during unbonding.Users are not refunded exit fees when cancelling withdrawing requests
Severity
- Severity: Low
≈
Likelihood: Medium×
Impact: Medium Submitted by
HickupHH3
Description
The refund amount upon cancelling a withdraw request is
sharesToUser
, which is after tolls, and is not refunded.Proof of Concept
function test_exitFee() public setRoles setNodesAndWeights setExitFee { vm.startPrank(user); vault.deposit{value: 1 ether}(user); uint256 userBalanceBefore = sHype.balanceOf(user); vault.requestUnlock(0.1 ether); // should have pulled the full share amount from the user uint256 userBalanceAfterRequest = sHype.balanceOf(user); assertEq(userBalanceAfterRequest, userBalanceBefore - 0.1 ether); // escrowed shares should have increased (uint40 bips, uint96 escrowedShares) = vault.exitFee(); assertEq(escrowedShares, 0.1 ether * bips / 10_000); // as escrowed shares have already been minted, shouldn't affect mintableProtocolShares() assertEq(vault.mintableProtocolShares(), 0); // if user cancels, will not be refunded exit fee vault.cancelUnlockRequest(0); uint256 userBalanceAfterCancel = sHype.balanceOf(user); uint256 expectedRefundAmount = 0.1 ether - escrowedShares; assertEq(userBalanceAfterCancel, userBalanceAfterRequest + expectedRefundAmount); vm.startPrank(privileged); vault.mintProtocolShares(receiver); // should have transferred escrowed shares to receiver assertEq(sHype.balanceOf(receiver), escrowedShares); assertEq(vault.mintableProtocolShares(), 0); // escrowedShares should have been zeroed (, escrowedShares) = vault.exitFee(); assertEq(escrowedShares, 0);}
Recommendation
Consider charging exit fees on redemptions.
Kintsu
Acknowledged, the approaches between applying fees on request vs. fees on redeem were weighed, and we opted to stick with the former.
- escrows fees as yield bearing sHYPE instead of native HYPE
- avoids advertising an incorrect UnlockRequest.spotValue
- withdrawing/accounting for 1 protocol assets (sHYPE) is preferred vs. 2 assets (sHYPE and HYPE)
- might require calling
sweep()
beforemintProtocolShares()
because escrowed HYPE could be redeemed by users - might get rid of
cancelUnlockRequest()
later
All of the above issues could be addressed but at the cost of tx gas and more critically contract runtime size.
Informational3 findings
contributeToPool() does not appreciating shares
Severity
- Severity: Informational
≈
Likelihood: Low×
Impact: Low Submitted by
cccz
Description
contributeToPool()
only increasesbatchDepositRequests.assets
but does not increasetotalPooled
, which results in contributions not appreciating shares.function contributeToPool(address benefactor) external payable { if (msg.value > type(uint96).max) revert DepositOverflow(); batchDepositRequests[currentBatchId].assets += uint96(msg.value); emit Contribution(msg.value, benefactor); }
Recommendation
Consider increasing
totalPooled
incontributeToPool()
.function contributeToPool(address benefactor) external payable { if (msg.value > type(uint96).max) revert DepositOverflow();+ totalPooled += uint96(msg.value); batchDepositRequests[currentBatchId].assets += uint96(msg.value); emit Contribution(msg.value, benefactor); }
Share dilution causes collected fees to be less than expected
State
- Acknowledged
Severity
- Severity: Informational
≈
Likelihood: High×
Impact: Low Submitted by
HickupHH3
Description
Note that the fees collected in
HYPE
will be less than expected because of share dilution.eg:
totalPooled == totalSupply = 10_000e18
,managementFee.bips = 1%
. Assume no activity for 1 year,totalPooled
has increased to11_000e18
from staking rewards, an increase of 10%. Hence, we expect the protocol to be entitled to 1% of11_000e18
=110e18
HYPE.The minted shares for the protocol is
10_000e18
* 1% =100e18
, bringing the total share supply to10_100e18
. When redeeming these shares, will be receiving100e18 / 10_100e18 * 11_000e18
=108.91e18
<110e18
HYPE.The formula to calculate the correct amount however requires tracking the profit, which seems difficult given the various moving parts + time weighted minting.
Recommendation
Since the affected party is the protocol, it is for informational purposes and is something to consider tweaking.
Minor Recommendations
Severity
- Severity: Informational
Submitted by
HickupHH3
Description & Recommendation
-
Registry.sol#L53: It would be convenient to have an external function with the same functionality, as there is a use-case where one wants to fetch information about a single node.
-
Registry.sol#L91: Increasing a deleted / non-existent node's weight will revert with
InvalidNode()
, not a no-op. -
Vault.sol#L605:
total_staked_after_phase1
should be in mixedCase- total_staked_after_phase1+ totalStakedAfterPhase1
-
The team made the following changes w.r.t cooldown variables for simplicity and greater clarity in commit 87fe056d