HoldStation - Audit

In the contract `HSStaking` the role `_owner` has authority over the functions shown in the diagram below.  Since the contract `HSStaking` inherits from `OwnableUpgradeable`, the `_owner` role has authority over the functions below as well: - `renounceOwnership`: Transfer ownership to address(0). - `transferOwnership`: Transfer ownership to the new owner's address. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority, set malicious account to the admin role. In the contract `HSStaking` the role `admin` has authority over the functions shown in the diagram below.  Any compromise to the `admin` account may allow the hacker to take advantage of this authority and do the following: - Update `unstakeNumberEpochs` to a large value to result in users being unable to withdraw their pledges - Update the reward rate results in yields from staking that deviate from expectations - Update the epoch duration to enable attackers to gain substantial profits in a short period --- In the contract `HSTradingVault` the role `_owner` has authority over the functions shown in the diagram below.  Since the contract `HSTradingVault` inherits from `OwnableUpgradeable`, the `_owner` role has authority over the functions below as well: - `renounceOwnership`: Transfer ownership to address(0). - `transferOwnership`: Transfer ownership to the new owner's address, which is overridden in the `HSTradingVault`, the new owner cannot be the current admin or manager address. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and do the following: - Set the `manager` role or transfer the ownership to malicious account - Update the maximum price delta on new epochs from open PNL to affect the staker's PNL - Update the interactive contracts into malicious contracts In the contract `HSTradingVault` the role `manager` has authority over the functions shown in the diagram below.  Any compromise to the `manager` account may allow the hacker to take advantage of this authority and do the following: - Set the `admin` role to malicious account - Update daily maximum minting amount for `HSToken` and vault shares - Update the ratio of profits used for destorying of `HSToken` - Update the `HSTokenPriceProvider` contarct to a malicious contract to manipulate the price of `HSToken` tokens - udpate max daily price delta from closed PNL In the contract `HSTradingVault` the role `pnlHandler` has authority over the functions below: - `sendAssets`: Withdraw `asset` from the vault and update the vault status. Any compromise to the `pnlHandler` account may allow the hacker to take advantage of this authority and withdraw `asset` tokens from the vault. Note: Based on design the `pnlHandler` should not be an EOA, it is supposed to be the `HSTradingCallbacks` which is not in the scope. In the contract `HSTradingVault` the role `openTradesPnlFeed` has authority over the functions below: - `updateAccPnlPerTokenUsed`: Updates `shareToAssetsPrice` based on the new `PnL` and starts a new epoch. Any compromise to the `openTradesPnlFeed` account may allow the hacker to take advantage of this authority and exploit by manipulating the share price.

The contracts `HSStaking` and `HSTradingVault` inherits upgradeable contracts, indicating that it is part of an upgradeable system. Upgradeable contracts often pair with a proxy contract that is responsible for managing contract upgrades. The `privileged roles` of the proxy often have the authority to update the implementation contracts. Any compromise of the privileged account could allow a hacker to exploit this authority, potentially altering the implementation contract pointed to by the proxy and thus executing malicious functionality within the implementation contract.

When a user deposits assets into the vault, the `totalDeposited` value is updated and the assets and shares have a specific ratio. Let's say if a reward is distributed to the vault in the same amount as the user's deposit, the shareToAssetsPrice doubles, allowing the user to withdraw double the assets. However, if the total value of assets withdrawn exceeds the totalDeposited amount, both the `withdraw()` and `redeem()` functions will revert due to the subtraction overflow. ```solidity=667 totalDeposited = isDeposit ? totalDeposited + assets : totalDeposited - assets; ```

The order of external call/transfer and storage manipulation must follow check effect interaction patterns. --- ```solidity=597 function distributeReward(uint256 assets) external { address sender = _msgSender(); SafeERC20Upgradeable.safeTransferFrom(_assetIERC20(), sender, address(this), assets); accRewardsPerToken += (assets * PRECISION) / totalSupply(); updateShareToAssetsPrice(); totalRewards += assets; emit RewardDistributed(sender, assets); } ``` --- ```solidity=633 function receiveAssets(uint256 assets, address user) external { address sender = _msgSender(); SafeERC20Upgradeable.safeTransferFrom(_assetIERC20(), sender, address(this), assets); uint256 assetsLessDeplete = assets; if (accPnlPerTokenUsed < 0 && accPnlPerToken < 0) { uint256 depleteAmount = (assets * lossesBurnP) / PRECISION / 100; assetsToDeplete += depleteAmount; assetsLessDeplete -= depleteAmount; } ``` --- ```solidity=696 require( dailyMintedHs <= (maxHsSupplyMintDailyP * IERC20Upgradeable(hsToken).totalSupply()) / PRECISION / 100, "ABOVE_INFLATION_LIMIT" ); address sender = _msgSender(); SafeERC20Upgradeable.safeTransferFrom(_assetIERC20(), sender, address(this), assets); int256 accPnlDelta = int256((assets * PRECISION) / supply); accPnlPerToken -= accPnlDelta; accPnlPerTokenUsed -= accPnlDelta; updateShareToAssetsPrice(); ``` --- ```solidity=121 function distributeRewardUsdc(uint256 _amount) external { usdc.transferFrom(_msgSender(), address(this), _amount); if (tokenBalance > 0) { accUsdcPerToken += (_amount * 1e18) / tokenBalance; totalRewardsDistributedUsdc += _amount; } emit UsdcDistributed(_amount); } ```

According to the logic of the function `makeWithdrawRequest`. If `tokenCredit` is a zero address and `msg.sender` is not the owner, `msg.sender` who gets the allowance from the `owner` can make a withdrawal request for the `owner`. ```solidity function makeWithdrawRequest(uint256 shares, address owner) external { require(shares > 0, "ZERO_VALUE"); require(openTradesPnlFeed.nextEpochValuesRequestCount() == 0, "END_OF_EPOCH"); address sender = _msgSender(); uint256 unlockEpoch = currentEpoch + withdrawEpochsTimelock(); if (address(tokenCredit) != address(0)) { require(sender == owner, "SENDER_MUSTBE_OWNER"); } else { require(sender == owner || (allowance(owner, sender) > 0 && allowance(owner, sender) >= shares), "NOT_ALLOWED"); } ... } ``` Besides, `msg.sender` who get the allowance from the `owner` can also redeem underlying asset on behalf of the `owner`. The related allowance will be deducted. ```solidity function redeem( uint256 shares, address receiver, address owner ) public override checks(shares, address(tokenCredit) != address(0)) returns (uint256) { if (address(tokenCredit) != address(0)) { require(_msgSender() == owner && _msgSender() == receiver, "SENDER_MUSTBE_OWNER"); } require(shares <= maxRedeem(owner), "ERC4626: redeem more than max"); withdrawRequests[owner][currentEpoch] -= shares; uint256 assets = previewRedeem(shares); scaleVariables(shares, assets, false); _withdraw(_msgSender(), receiver, owner, assets, shares); if (address(tokenCredit) != address(0)) { tokenCredit.notifyBalanceChange(_msgSender(), address(0)); } emit Redeemed(receiver, assets, currentEpoch); return assets; } ```

According to the design of `HSStaking`, stakers can get `esHoldRewards` if they have staked tokens in the contract. The reward state will be updated when users staking, unstaking and claiming reward. ```solidity UserReward storage userReward = esHoldRewards[staker][currentEpoch + 1]; userReward.total = u.stakedTokens; userReward.isEmpty = userReward.total > 0 ? false : true; ``` Besides, based on the logic of the function `calculateEsHoldReward()`, epochs in which reward updates are not triggered will default to using the rewards from the previous update. ```solidity for (uint i = claimedAtEpoch[_staker]; i <= currentEpoch; i++) { UserReward memory userReward = esHoldRewards[_staker][i]; if (userReward.isEmpty) { lastChange = 0; continue; } if (userReward.total > 0) { lastChange = userReward.total; } totalReward += ((lastChange * epochRewards[i]) / PERCENTAGE_BASE); } ``` At last, epoch can only be increased by calling the function `forceNewEpoch()`. If the function is not called for an extended period, the epoch will be prolonged. This implies that each epoch's time period may not be equal.

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. --- ```solidity=376 return (( collatP <= 100 * PRECISION ? maxDiscountP : collatP <= maxDiscountThresholdP ? (maxDiscountP * (maxDiscountThresholdP - collatP)) / (maxDiscountThresholdP - 100 * PRECISION) : 0 ) * lockDuration) / MAX_LOCK_DURATION; ``` --- ```solidity=554 totalLiability += ((int256(shares) * totalLiability) / int256(supply)) * (isDeposit ? int256(1) : int256(-1)); ```

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user stakes 100 deflationary tokens (with a 10% transaction fee) in a staking contract, only 90 tokens actually arrive in the contract. However, the user can still withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. As a result, the following token contract involved in the `HSStaking` will need extra validation if any of them is a deflationary token: - `token`, if `token` is the deflationary token, the amount of staking should be revised. - `usdc`, if `usdc` is the deflationary token, the amount of pending reward should be revised. Similarly, in the contract `HSTradingVault`, the following token contract will need validation if a deflationary token implied: - `asset` token for ERC4626

In the `HSStaking` contract, the `setTimelock()` and `setUnstakeNumberEpochs()` functions lack sanity checks. ```solidity function setTimelock(uint _timelock) external onlyAdmin { timelock = _timelock; emit TimelockUpdated(_timelock); } function setUnstakeNumberEpochs(uint _value) external onlyAdmin { unstakeNumberEpochs = _value; emit UnstakeNumberEpochs(_value); } ``` When `timelock` is set to 0, anyone can call `forceNewEpoch()` without restrictions, spanning multiple epochs, and then call `claimESHoldReward()` to obtain a significant amount of `rewardToken`. When `unstakeNumberEpochs` is set to 0, stakers can retrieve their staking at any time, which exist potential risks. An attacker monitoring the `distributeRewardUsdc()` transaction could make substantial stakes before the rewards are distributed to the contract and swiftly unstake to capture a significant portion of the rewards.

The `HSTradingVault` is initialized via `initialize` function. So, all inherited functions' initialization functions should be invoked. However, the inherited contract `OwnableUpgradeable` is not initialized in the current function `initialize`.