Ventuals

vHYPE Staking

6.1 Exchange Rate Manipulation Leading to Inflation Attack

The HYPETovHYPE function in the GenesisVaultManager contract is vulnerable to an inflation attack due to exchange rate manipulation. The exchange rate is calculated as totalHYPE / totalVHYPE, which can be artificially inflated by donating directly to the vault without minting corresponding shares.

An attacker can exploit this by: (1) depositing a minimal amount (1 wei) to receive 1 vHYPE share, (2) donating a large amount (e.g., 10 ETH) directly to the staking vault via the receive() function, (3) inflating the exchange rate to approximately (10 ETH * 1e18) / 1 wei ≈ 10^22, and (4) causing victim deposits to result in 0 vHYPE shares due to integer division truncation when hypeAmount * 1e18 < _exchangeRate in the Math.mulDiv(hypeAmount, 1e18, _exchangeRate) calculation.

This allows the attacker to gain control of all vault assets with minimal investment while legitimate depositors receive 0 vHYPE shares despite depositing significant amounts, resulting in a complete loss of funds for victims and making the vault unusable for legitimate users.

Recommendation

Use internal accounting to keep track of all the HYPE in the vault with a storage variable and update it when the contract receives HYPE instead of using .balance.

Resolution

Fixed by 9007e61d16 and f5da5068c2

6.2 Not burning vHype on transfer

On depositing Hype tokens into the GenesisVaultManager contract, the tokens are moved into the StakingVault. At this time, the vHype tokens are minted for the accepted amount.

From the StakingVault, the manager can transfer Hype to any other account by calling the transferHype function, which will move the funds out of the protocol.

While Hype tokens are taken out of the protocol, the extent of vHype is not burnt.

While computing the exchange rate, the totalBalance for Hype comprises staking Balance + Spot Balance + funds in the StakingVault contract.

function exchangeRate() public view returns (uint256) {
==>  uint256 balance = totalBalance();
==>  uint256 totalSupply = vHYPE.totalSupply();
   //....
   return Math.mulDiv(balance, 1e18, totalSupply);
}

As the vHype was not burnt when taking out funds from StakingVault, it will directly impact the exchange rate.

Recommendation

Consider removing the transferHype function or burn equivalent vHYPE shares on transfer.

Resolution

Removed the transferHype function a02ad2aa62

6.3 Decimal Conversion Loss for Deposited Amounts

The system suffers from decimal conversion loss when handling HYPE deposits due to the conversion between 18 decimals (EVM standard) and 8 decimals (HyperCore standard).

The Converters.to8Decimals() function performs a division by 1e10 to convert from 18 decimals to 8 decimals: This division operation truncates any remainder less than 1e10 wei, resulting in permanent loss of precision. For example:

• A deposit of 1.0000000001 HYPE (1e18 + 100 wei) would be converted to 1 HYPE (1e8) on HyperCore;
• The 100 wei remainder is permanently lost;
• When converted back to 18 decimals, it becomes 1.0000000000 HYPE (1e18), losing the original 100 wei;

Recommendation

Enforce minimum deposit granularity by a validation in the deposit() function to ensure only multiples of the minimum precision unit can be deposited (amountToDeposit % 1e10 == 0). Besides, add a minimum deposit amount check to prevent dust deposits that would result in significant relative precision loss.

Resolution

Added minimum deposit constraint 9007e61d1

6.4 Manager role to be restricted to GenesisVaultManager

In the StakingVault there are functions that interact with HyperCore. For the actions performed on HyperCore, the updated state does not reflect during reads via pre-compiled contracts until the next block. Hence, any read before the formation of the next block could result in incorrect values.

Currently, in order to prevent such a read, the tracking variable for the last action is in the GenesisVaultManager contract. If the manager role is configured for GenesisVaultManager and another EOA account, there is a possibility for EOA to interact with HyperCore in the StakingVault, resulting in changes to HyperCore state in transit.

For example, considering HYPE can be deposited directly into the StakingVault, if the intent is to send those funds to any of the below actions interacting with HyperCore, it will impact the working of the protocol.

function tokenDelegate(address validator, uint64 weiAmount, bool isUndelegate) external onlyManager whenNotPaused {
     CoreWriterLibrary.tokenDelegate(validator, weiAmount, isUndelegate);
 }


 /// @inheritdoc IStakingVault
 function spotSend(address destination, uint64 token, uint64 weiAmount) external onlyManager whenNotPaused {
     CoreWriterLibrary.spotSend(destination, token, weiAmount);
 }


 /// @inheritdoc IStakingVault
 function transferHypeToCore(uint256 amount) external onlyManager whenNotPaused {
     _transfer(payable(HYPE_SYSTEM_ADDRESS), amount);
 }

As the protection implemented in GenesisVaultManager is not effective in StakingVault, it could result in incorrect exchange rates.

Recommendation

Consider enforcing the Manager role to GenesisVaultManager during the genesis phase which is the intent. An improvement to consider is to move the _transferToCoreAndDelegate internal function into StakingVault so that interactions between stakingVault and HyperCore will be effective even after the genesis phase.

Resolution

As discussed, moved the lastEvmToCoreTransferBlockNumber logic to StakingVault so that the one-block delay is enforced in the StakingVault. 6c4f718eff

6.5 Hype tokens deposited directly into GenesisVault Manager are stuck

GenesisVaultManager can accept Hype tokens directly as the contract implements receive and fallback functions. While the Hype accepted via deposit function, equivalent vHype is minted and tokens are moved to the stakingVault, the directly deposited will remain in the GenesisVaultManager contract.

As there is no function to withdraw the hype tokens from GenesisVaultManager contract, any deposits directly sent to the contract will be stuck and locked forever.

Recommendation

Consider removing the receive and fallback functions

Resolution

Removed the receive and fallback functions 0d6903e09914

6.6 Malicious user can inflate the price of vHype

User can deposit Hype tokens into GenesisVaultManager contract, against which he will receive VHYPE tokens. The user can burn his vHYPE tokens at his discretion by calling burn on the Token contract.

Burning the token will impact the totalSupply as the supply will reduce to the extent of tokens burnt. This will directly inflate the value of VHYPE tokens.

Recommendation

Consider removing the burn function.

Resolution

Not a bug per-se, since if a user intentionally burned their tokens, the exchange rate *should* get inflated. There isn't a way for an attacker to exploit this, since they would just end up losing their claim on the underlying HYPE in the net.

However, we discussed removing this burn function to avoid accidental burns and reduce the surface area of bugs. Removed in this PR 5d78a553ac7

6.7 _mint() can be called when the contract is paused

The Base contract has a whenNotPaused modifier that is used to prevent certain actions when the contract is paused. The VHYPE contract inherits from Base and has a mint function. The mint function does not have whenNotPaused and hence VHYPE tokens can be minted even when VHYPE contract is paused.

function mint(address to, uint256 amount) public onlyManager {
   _mint(to, amount);
}

Recommendation

Consider adding the whenNotPaused modifier to the mint function. This would prevent new tokens from being minted when the contract is paused, ensuring consistent behavior across both public and internal minting methods.

Resolution

Added whenNotPaused to mint() 5d4a5c7e88d

6.8 Event log can be cluttered with 0 deposits

The deposit function can be called with 0 HYPE. As the deposit function does not check for msg.value to be greater than 0, the function will execute and generate a deposit event while consuming gas.

function deposit() public payable canDeposit whenNotPaused {
       uint256 requestedDepositAmount = msg.value;// if msg.value is 0
       //...


==>     emit Deposit(
           msg.sender, /* depositor */
           amountToMint, /* minted */
           amountToDeposit, /* deposited */
           requestedDepositAmount - amountToDeposit /* refunded */
       );
   }

Recommendation

Consider adding a validation for msg.value to be greater than 0 or preferably a minimum deposit amount.

Resolution

Fixed by minimum deposit limits 9007e61d162

6.9 Inconsistent comment for weiAmount conversion in CoreWriterLibrary

The CoreWriterLibrary contract contains an inconsistent comment that states that weiAmount should be converted to 6 decimals before being passed to its functions. However, the rest of the protocol, including the GenesisVaultManager contract and the function-level Natspec comments, consistently uses 8 decimals for HYPE amounts sent to HyperCore. This discrepancy can lead to confusion.

The misleading comment is found in the contract-level NatSpec as below.

/// All weiAmounts must be converted to 6 decimals before calling these functions.
// ...
library CoreWriterLibrary {
   // ...
}

Recommendation

Consider updating the CoreWriterLibrary contract's NatSpec comment to reflect the correct decimal conversion

Resolution

Fixed 608e7fd69e4

6.10 emergencyStakingWithdraw may fail

In the GenesisVaultManager contract, the funds moved to the HyperCore are delegated to a defaultVault.

The delegations to a particular validator have a lockup period of 1 day. Only after this lockup, delegations may be partially or fully undelegated at any time.

In the current implementation, the funds are delegated to the defaultVault.

If there arises a scenario where there is a need for emergencyStakingWithdraw, the earliest undelegate request can be submitted only after passing of 1 day from the last deposit.

function emergencyStakingWithdraw(uint256 amount, string calldata purpose) external onlyOwner {
      //...


==>   // Immediately undelegate HYPE
      // @audit, the undelagate can be called 1 day after
      // the last deposit.
       stakingVault.tokenDelegate(defaultValidator, amount.to8Decimals(), true);


     //...
   }

Recommendation

Consider error handling in case emergencyStakingWithdraw does not work due to recent delegation to the validator.

Resolution

Addressed in this f045057de3

6.11 whitelistDepositLimits should be greater than current defaultDepositLimit

When setting the whitelistDepositLimit for an address, the limit should be greater than the current defaultDepositLimit.

function setDefaultDepositLimit(uint256 _defaultDepositLimit) public onlyOwner {
   defaultDepositLimit = _defaultDepositLimit;
}


function setWhitelistDepositLimit(address depositor, uint256 limit) public onlyOwner {
   whitelistDepositLimits[depositor] = limit;
}

Recommendation

Consider adding a validation to check the limit to be greater than defaultDepositLimit.

Resolution

Going to leave behavior as-is, but updated the comment to avoid confusion 19c95b5798

6.12 Upgradeable contract lacks storage gaps

The protocol implements a Base contract using UUPS upgradeable proxy pattern. The common functionality is implemented in the Base contract, which is extended by other contracts in the protocol.

As the protocol follows the traditional storage for state, the storage layout is defined. In the future, if the Base contract needs to be extended to support more common features with state, it will not be possible. The absence of storage gaps removes the flexibility to insert variables in future upgrades without causing storage collisions.

Recommendation

Consider adding storage gaps to provision future extension in functionality.

Resolution

Fixed 0231aee978de

Issues in the new version

6.13 Withdrawals can be claimed before finalisation

The function claimWithdraw() performs following checks:

function claimWithdraw(uint256 withdrawId, address destination) public whenNotPaused {
        Withdraw storage withdraw = withdraws[withdrawId];
        require(msg.sender == withdraw.account, NotAuthorized());
        require(!withdraw.cancelled, WithdrawCancelled());
        require(!withdraw.claimed, WithdrawClaimed());

        Batch memory batch = batches[withdraw.batchIndex];
        require(block.timestamp > batch.finalizedAt + 7 days, WithdrawUnclaimable()); // TODO: Should we add a buffer?

        uint256 withdrawExchangeRate = batch.slashed ? batch.slashedExchangeRate : batch.snapshotExchangeRate;
        uint256 hypeAmount = _vHYPEtoHYPE(withdraw.vhypeAmount, withdrawExchangeRate);
        ...
}

The function, however, does not check if the withdraw.batchIndex is not type(uint256).max. Also, the requirement block.timestamp > batch.finalizedAt + 7 days does not take into consideration that batch.finalizedAt with value 0 would pass. When the request is processed but the batch is not finalised, the request would contain the current batchIndex and the batch would contain snapshotExchangeRate, but the finalizedAt would be 0. This allows users to claim funds at the snapshotExchangeRate without the 7-day waiting period. In consequence, the calculations in the protocol would be incorrect, leading to loss of funds.

Recommendation

Consider preventing claiming in waiting period by checking if the withdraw.batchIndex is not type(uint256).max and that batch.finalizedAt is not 0.

Resolution

Fixed 8def0bb

6.14 Lack of pause for burnFrom()

The burnFrom() function in VHYPE does not contain the pause check.

Recommendation

Consider introducing the pause check to VHYPE.burnFrom().

Resolution

Fixed 92b7aeb

6.15 Lack of withdrawal request minimum amount can lead to DOS

The function queueWithdraw() allows user to request withdrawal of any amount of vHYP greater than 0. A malicious user can create requests with a small amount of vHYPE to temporarily DOS the batch finalisation. The finalisation can only be done if there are no withdrawal requests in the queue that could be fulfilled with available funds, which is checked in _canFinalizeBatch(). A malicious user could create a withdrawal request with a small amount before the finalisation, to add another request that can be fulfilled, effectively stopping the finalisation. Moreover, users could add many small-value requests to disincentivise calling processBatch() due to high gas cost.

Recommendation

Consider defining minimum withdrawal request value.

Resolution

Fixed 10e585a and 0847334

6.16 Missing Validator address validation

The stake and tokenRedelegate functions do not validate validator addresses. They will not fail with validator == address(0) or any invalid address, which could lead to silent errors since the functions will not revert but may fail silently during delegation operations.

Recommendation

Add validation to ensure validator addresses are not zero addresses and consider implementing a whitelist of allowed validators.

Resolution

Fixed 39e9bc8

6.17 No limit on withdrawal request

Users can request withdrawal of any amount. The requests are processed in a queue. Withdrawn amount is limited by the HyperCore staking restriction, at least 500k HYPE tokens (initially) must be staked to maintain the perpetuals DEX deployment. This is checked in StakingVaultManager with minimumStakeBalance in the processBatch() function. However, there is a scenario where a single user requests a huge amount of tokens to withdraw. In such a case, there must be enough funds available to process the request of that user and all the subsequent requests. If that amount is really high, the withdrawals may be blocked for a substantial amount of time.

Recommendation

Consider introducing maximum limit for withdrawal request. This would incentivise users to withdraw in smaller portions and allow continuous withdrawals.

Resolution

Fixed 582641b. The fix is implemented such that we will automatically split a large withdraw request into smaller withdraws

6.18 Problems with applySlash() after claiming withdrawals

When the slashing happens, admins of the vHype protocol call the applySlash(). However, if this function is applied too late, users can claim the withdrawal funds from the affected batches. This may result in two problems:

• users can claim their withdrawals at the original (higher) exchange rate before slashing is applied, allowing them to circumvent slashing penalties;
• during the claiming the totalHypeClaimed is increased, but the applySlash() adjusts only the totalHypeProcessed according to the new slashedExchangeRate. This may result in totalHypeClaimed > totalHypeProcessed which would cause revery in totalBalance() since reservedHypeForWithdraws = totalHypeProcessed - totalHypeClaimed would result in underflow;

Note that the expected scenario is that the admins would react immediately to the slash event and call applySlash() only on the affected batches.

Recommendation(s): To ensure that no claiming happens between the actual slash event and calling applySlash(), consider introducing a two-step mechanism, which would first apply the slash or not, and only after allow claiming.

Resolution

Fixed ee2d169

6.19 Truncation during HyperEVM to HyperCore transfer

The truncation is not applied during the transfer of funds to the spot account in finaliseBatch(). This results in potential loss of 1e10 wei during every deposit.

function finalizeBatch() external whenNotPaused whenBatchProcessingNotPaused {
      ...

        // Always transfer the full deposit amount to HyperCore spot
        if (depositsInBatch > 0) {
            stakingVault.transferHypeToCore(depositsInBatch);
        }
        ...
}

The depositsInBatch will be truncated during transfer to HyperCore, but the whole un-truncated amount will be deducted from the StakingVault.

Recommendation

Consider applying restriction of precision less or equal to 1e8. This can be applied by adding a check to enforce amount % 1e10 == 0.

Resolution

Fixed 66961ad

6.20 Unclear error message when claiming unprocessed withdraw

When claimWithdraw is called for a withdrawal that hasn't been processed yet (batchIndex is still type(uint256).max), the function doesn't fail early with a clear error message. Instead, it proceeds through all validation checks and only fails when reaching spotSend() because the balance will be 0, resulting in an unclear error message that doesn't indicate the withdrawal hasn't been processed yet.

Recommendation

Add explicit validation to check that withdraw.batchIndex != type(uint256).max in claimWithdraw and provide a clear error message if the withdrawal hasn't been processed yet.

Resolution

Fixed 8def0bb

6.21 Validation logic should be moved to StakingVault

The validation checks for Core user existence and spot balance are performed in StakingVaultManager::claimWithdraw() before calling StakingVault::spotSend(). These checks should be moved to the StakingVault::spotSend() function to ensure that if/when StakingVaultManager is upgraded, the essential validation checks are still in place since they are critical for StakingVault functionality.

Recommendation

Move the Core user existence and spot balance validation checks to the StakingVault::spotSend() function to centralize validation logic.

Resolution

Fixed 9b9cd8a and 3bc3b57

6.22 Unclear timing logic in delegation reading

The timing constraint logic uses block.number > lastDelegationChangeBlockNumber[_validator] + 1 in the block number comparison, but it's unclear why this additional block is required. This creates inconsistency with other timing checks in the contract that don't use + 1 like in the deposit function, where we have block.number > lastEvmToCoreTransferBlockNumber.

Recommendation

Clarify the reasoning for the + 1 requirement in the documentation or remove it for consistency.

Resolution

Fixed b59f8a9

6.23 minimumStakeBalance changes can temporarily block batch finalization

Increasing minimumStakeBalance after processing a batch but before finalization can temporarily prevent batch finalization if the new minimum makes balance < minimumStakeBalance + hypeProcessed. However, this can be resolved by lowering minimumStakeBalance again or using the resetBatch function.

Recommendation

Consider adding a check in the setMinimumStakeBalance function to ensure that the new minimum is not greater than the current balance if a batch is already processing.

Resolution

Fixed 65102fd

6.24 Increase timing checks

There are two time checks in the manager: (1) a 7-day withdrawal queue and (2) a 1-day unstake lockup after stake. Those checks mirror the mechanisms on HyperCore. It is recommended to increase those checks slightly to ensure that the calls won't result in silent failures, which may cause loss of funds.

Recommendation

Consider increasing time checks to ensure correct behaviour. Additionally, consider increasing the block separation time to be greater than 1.

Resolution

Fixed d7fea7e