From cf526b1160696b5384768d537873529cf53c4c1f Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Thu, 12 Dec 2024 10:43:01 +0200 Subject: [PATCH 01/15] add notes --- contracts/HydraChain/HydraChain.sol | 6 ++++++ .../modules/DaoIncentive/DaoIncentive.sol | 2 ++ .../modules/Inspector/Inspector.sol | 8 ++++++-- .../ValidatorManager/ValidatorManager.sol | 3 +++ .../modules/ValidatorsData/ValidatorsData.sol | 2 ++ .../DelegationPoolLib/DelegationPoolLib.sol | 1 - .../VestedDelegation/VestedDelegation.sol | 2 -- contracts/HydraStaking/HydraStaking.sol | 20 +++++++++++++------ contracts/HydraStaking/Staking.sol | 3 ++- .../DelegatedStaking/DelegatedStaking.sol | 1 + .../modules/VestedStaking/VestedStaking.sol | 2 ++ contracts/common/Liquid/Liquid.sol | 3 ++- 12 files changed, 40 insertions(+), 13 deletions(-) diff --git a/contracts/HydraChain/HydraChain.sol b/contracts/HydraChain/HydraChain.sol index 39c03cad..0dffb3ff 100644 --- a/contracts/HydraChain/HydraChain.sol +++ b/contracts/HydraChain/HydraChain.sol @@ -13,14 +13,17 @@ import {IHydraChain} from "./IHydraChain.sol"; import {Epoch} from "./IHydraChain.sol"; contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { + // @note unsafe arrays are used in the code using ArraysUpgradeable for uint256[]; uint256 public currentEpochId; + // @note this is unsafe array /// @notice Array with epoch ending blocks uint256[] public epochEndBlocks; /// @notice Epoch data linked with the epoch id mapping(uint256 => Epoch) public epochs; + // @audit isn't this the same as the epochEndBlocks? mapping(uint256 => uint256) internal _commitBlockNumbers; // _______________ Initializer _______________ @@ -68,6 +71,7 @@ contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { if (epoch.startBlock >= epoch.endBlock) { revert CommitEpochFailed("NO_BLOCKS_COMMITTED"); } + // @audit this is going to be a problem when slashing is added if ((epoch.endBlock - epoch.startBlock + 1) % epochSize != 0) { revert CommitEpochFailed("EPOCH_MUST_BE_DIVISIBLE_BY_EPOCH_SIZE"); } @@ -145,8 +149,10 @@ contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { uint256 validatorParticipation = validatorsParticipation[validator]; // check if the validator is active and the last participation is less than the threshold if ( + // @audit here we check if the validator is active but this doesn't mean it is actually part of the active set (check if this is breakable) validators[validator].status == ValidatorStatus.Active && lastCommittedEndBlock > validatorParticipation && + // @audit at block 1 what is the value of validatorParticipation? lastCommittedEndBlock is 0 in the first 500 blocks lastCommittedEndBlock - validatorParticipation >= initiateBanThreshold ) { return true; diff --git a/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol b/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol index a3826c31..88276a3b 100644 --- a/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol +++ b/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol @@ -39,6 +39,7 @@ abstract contract DaoIncentive is IDaoIncentive, System, Initializable, RewardWa * @inheritdoc IDaoIncentive */ function distributeDAOIncentive() external onlySystemCall { + // @audit put all multiplications before division to avoid rounding errors uint256 reward = (((hydraStakingContract.totalBalance() * 200) / 10000) * (block.timestamp - lastDistribution)) / 365 days; lastDistribution = block.timestamp; @@ -51,6 +52,7 @@ abstract contract DaoIncentive is IDaoIncentive, System, Initializable, RewardWa * @inheritdoc IDaoIncentive */ function claimVaultFunds() external { + // @audit reward can be renamed to incentive or something uint256 reward = vaultDistribution; if (reward == 0) { revert NoVaultFundsToClaim(); diff --git a/contracts/HydraChain/modules/Inspector/Inspector.sol b/contracts/HydraChain/modules/Inspector/Inspector.sol index 0a644f59..346e869c 100644 --- a/contracts/HydraChain/modules/Inspector/Inspector.sol +++ b/contracts/HydraChain/modules/Inspector/Inspector.sol @@ -12,9 +12,13 @@ abstract contract Inspector is IInspector, ValidatorManager { uint256 public validatorPenalty; /// @notice The reward for the person who reports a validator that have to be banned uint256 public reporterReward; - /// @notice Validator inactiveness (in blocks) threshold that needs to be passed to initiate ban for a validator + /// @notice Validator inactiveness (in blocks) threshold + /// that needs to be reached or passed to initiate ban for a validator + /// @dev must be always bigger than the epoch length (better bigger than at least 4 epochs), + /// otherwise all validators can be banned uint256 public initiateBanThreshold; - /// @notice Validator inactiveness (in seconds) threshold that needs to be passed to ban a validator + /// @notice Validator inactiveness (in seconds) threshold. + /// A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. uint256 public banThreshold; /// @notice Mapping of the validators that bans has been initiated for (validator => timestamp) mapping(address => uint256) public bansInitiated; diff --git a/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol b/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol index bd3d3ed7..13596228 100644 --- a/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol +++ b/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol @@ -36,6 +36,7 @@ abstract contract ValidatorManager is mapping(address => Validator) public validators; /** * @notice Mapping that keeps the last time when a validator has participated in the consensus + * @dev Updated on epoch-ending blocks only * @dev Keep in mind that the validator will initially be set active when stake, * but it will be able to participate in the next epoch. So, the validator will have * less blocks to participate before getting eligible for ban. @@ -74,11 +75,13 @@ abstract contract ValidatorManager is // _______________ Modifiers _______________ + // @audit unused modifier modifier onlyActiveValidator(address validator) { if (validators[validator].status != ValidatorStatus.Active) revert Unauthorized("INACTIVE_VALIDATOR"); _; } + // @audit unused modifier /// @notice Modifier to check if the validator is registered or active modifier onlyValidator(address validator) { if ( diff --git a/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol b/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol index d42a9de8..9a95c663 100644 --- a/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol +++ b/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol @@ -26,9 +26,11 @@ abstract contract ValidatorsData is IValidatorsData, System, Initializable { return; } + // @audt you were able to use a single int256 variable to store the changes in the power uint256 totalNewPower = 0; uint256 totalOldPower = 0; for (uint256 i = 0; i < arrLength; i++) { + // @audit naming is confusing totalNewPower += validatorsPower[i].votingPower; totalOldPower += validatorPower[validatorsPower[i].validator]; validatorPower[validatorsPower[i].validator] = validatorsPower[i].votingPower; diff --git a/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol b/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol index 62103f8f..576ffcb5 100644 --- a/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol +++ b/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol @@ -245,7 +245,6 @@ library DelegationPoolLib { * @param currentEpochNum the current epoch number * @return bool whether the balance change is made */ - // TODO: Check if the commitEpoch is the last transaction in the epoch, otherwise bug may occur function isBalanceChangeMade( DelegationPool storage pool, address delegator, diff --git a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol index 641e437a..ece467cd 100644 --- a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol +++ b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol @@ -202,7 +202,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve delegation.cleanDelegatorHistoricalData(msg.sender); uint256 duration = durationWeeks * 1 weeks; - // TODO: calculate end of period instead of write in the cold storage. It is cheaper vestedDelegationPositions[staker][msg.sender] = VestingPosition({ duration: duration, start: block.timestamp, @@ -341,7 +340,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve // _______________ Public functions _______________ - // TODO: Check if the commitEpoch is the last transaction in the epoch, otherwise bug may occur /** * @inheritdoc IVestedDelegation */ diff --git a/contracts/HydraStaking/HydraStaking.sol b/contracts/HydraStaking/HydraStaking.sol index 3b5fac13..e59b5caa 100644 --- a/contracts/HydraStaking/HydraStaking.sol +++ b/contracts/HydraStaking/HydraStaking.sol @@ -14,8 +14,6 @@ import {PenalizeableStaking} from "./modules/PenalizeableStaking/PenalizeableSta import {IHydraStaking, StakerInit} from "./IHydraStaking.sol"; import {Staking, IStaking} from "./Staking.sol"; -// TODO: An optimization we can do is keeping only once the general apr params for a block so we don' have to keep them for every single user - contract HydraStaking is IHydraStaking, System, @@ -29,7 +27,8 @@ contract HydraStaking is { using SafeMathUint for uint256; - uint256 public lastDistribution; // last rewards distribution timestamp + /// @notice last rewards distribution timestamp + uint256 public lastDistribution; /// @notice Mapping used to keep the paid rewards per epoch mapping(uint256 => uint256) public distributedRewardPerEpoch; @@ -105,6 +104,7 @@ contract HydraStaking is /** * @inheritdoc IHydraStaking */ + // @audit ensure user can't stake unstake and delegations as well, so that balances changed is not invoked function temporaryEjectValidator(address account) external onlyHydraChain { emit BalanceChanged(account, 0); } @@ -180,6 +180,7 @@ contract HydraStaking is } } + // @audit this function can be removed and just using the main _unstake func; beforePenalizeStakeHook must be added to fix liquidity _unstake problem function _executeUnstake( address staker, uint256 unstakeAmount @@ -193,7 +194,7 @@ contract HydraStaking is * @inheritdoc PenalizeableStaking */ function _afterPenalizeStakerHook(address staker, uint256 unstakeAmount, uint256 leftForStaker) internal override { - // the unstake amount of liquid tokens must be paid at the time of withdrawal + // the unstake amount of liquid tokens must be paid at the time of initiatePenalizedFundsWithdrawal // but only the leftForStaker will be automatically requested, // so we have to set the unstake amount - leftForStaker as liquidity debt that must be paid as well liquidityDebts[staker] += (unstakeAmount - leftForStaker).toInt256Safe(); @@ -233,10 +234,14 @@ contract HydraStaking is */ function _distributeTokens(address staker, uint256 amount) internal virtual override { VestingPosition memory position = vestedStakingPositions[staker]; + // This check works because if position has already been opened, the restrictions on stake() and stakeWithVesting() + // will prevent entering the check again if (_isOpeningPosition(position)) { uint256 currentStake = stakeOf(staker); if (currentStake != amount) { currentStake -= amount; + // We want all previously distributed tokens from normal stake() to be collected, + // because for vested positions we distribute decreased amount of liquid tokens _collectTokens(staker, currentStake); amount += currentStake; } @@ -266,7 +271,7 @@ contract HydraStaking is /** * @notice Distributes the reward for the given staker. - * @notice Validator won't receive a reward in the epoch of exiting his position (stake becomes 0). His delegators will receive a reward for his uptime. + * @notice Validator won't receive a reward in the epoch of exiting the staking (stake becomes 0). His delegators will receive a reward for his uptime. * @param epochId The epoch id * @param uptime The uptime data for the validator (staker) * @param fullRewardIndex The full reward index @@ -282,6 +287,7 @@ contract HydraStaking is uint256 totalBlocks ) private returns (uint256 reward) { if (uptime.signedBlocks > totalBlocks) { + // @audit if a bug arrises in the node, this revert ca stop the whole distribution process. Better in such case set the signedBlocks to totalBlocks revert DistributeRewardFailed("SIGNED_BLOCKS_EXCEEDS_TOTAL"); } @@ -296,6 +302,7 @@ contract HydraStaking is stakerRewardIndex ); + // @audit isn't better check if stakerShares is 0? Same for delegation _distributeStakingReward(uptime.validator, stakerShares); _distributeDelegationRewards(uptime.validator, delegatorShares, epochId); @@ -318,6 +325,7 @@ contract HydraStaking is uint256 delegatedBalance, uint256 totalReward ) private pure returns (uint256, uint256) { + // @audit what happens if both stakedBalance and delegatedBalance are 0? the reward will be lost. Is that okay? if (stakedBalance == 0) return (0, totalReward); if (delegatedBalance == 0) return (totalReward, 0); uint256 stakerReward = (totalReward * stakedBalance) / (stakedBalance + delegatedBalance); @@ -330,7 +338,7 @@ contract HydraStaking is * Calculates the epoch reward index. * We call it index because it is not the actual reward * but only the macroFactor and the "time passed from last distribution / 365 days ratio" are applied here. - * we need to apply the ration because all APR params are yearly + * we need to apply the ratio because all APR params are yearly * but we distribute rewards only for the time that has passed from last distribution. * The participation factor is applied later in the distribution process. * (base + vesting and RSI are applied on claimReward for delegators diff --git a/contracts/HydraStaking/Staking.sol b/contracts/HydraStaking/Staking.sol index c3ca82f0..7916a4e0 100644 --- a/contracts/HydraStaking/Staking.sol +++ b/contracts/HydraStaking/Staking.sol @@ -110,6 +110,7 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn * @param amount The amount to stake */ function _stake(address account, uint256 amount) internal virtual { + // @audit move check to HydraStaking._stake if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); uint256 currentBalance = stakeOf(account); @@ -131,6 +132,7 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn address account, uint256 amount ) internal virtual returns (uint256 stakeLeft, uint256 withdrawAmount) { + // @audit move check to HydraStaking._unstake if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); uint256 accountStake = stakeOf(account); @@ -151,7 +153,6 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn /** * @notice Function that calculates the end reward for a user (without vesting bonuses) based on the pool reward index. - * @dev Denominator is used because we should work with floating-point numbers * @param rewardIndex index The reward index that we apply the base APR to * @dev The reward with the applied APR */ diff --git a/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol b/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol index 31fe5c61..efacad56 100644 --- a/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol +++ b/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol @@ -84,6 +84,7 @@ abstract contract DelegatedStaking is IDelegatedStaking, Staking { * @return The total amount of delegation */ function _totalDelegation() internal view returns (uint256) { + // @audit check of total delegation is properly changed on ban procedures and similar return delegationContract.totalDelegation(); } diff --git a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol index 13d69956..5a2c5476 100644 --- a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol +++ b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol @@ -47,6 +47,7 @@ abstract contract VestedStaking is IVestedStaking, Vesting, Staking { revert StakeRequirement({src: "stakeWithVesting", msg: "ALREADY_IN_VESTING_CYCLE"}); } // Claim the rewards before opening a new position, to avoid locking them during vesting cycle + // @audit this claim function doesn't actually claim the rewards. Bug!!! if (unclaimedRewards(msg.sender) != 0) _claimStakingRewards(msg.sender); // Clear the staking rewards history @@ -133,6 +134,7 @@ abstract contract VestedStaking is IVestedStaking, Vesting, Staking { /** * @inheritdoc IVestedStaking */ + // @audit this function returns wrong data for reward function calcVestedStakingPositionPenalty( address staker, uint256 amount diff --git a/contracts/common/Liquid/Liquid.sol b/contracts/common/Liquid/Liquid.sol index 3071f481..2163957f 100644 --- a/contracts/common/Liquid/Liquid.sol +++ b/contracts/common/Liquid/Liquid.sol @@ -59,9 +59,10 @@ abstract contract Liquid is ILiquid, Initializable { int256 liquidDebt = liquidityDebts[account]; int256 amountInt = amount.toInt256Safe(); int256 amountAfterDebt = amountInt + liquidDebt; - // if negative debt is bigger than or equal to the amount, so we get the whole amount from the debt + // if negative debt is bigger than or equal to the amount, we get the whole amount from the debt if (amountAfterDebt < 1) { liquidityDebts[account] -= amountInt; + // @audit unused variable amount = 0; return; From 05346ccf6f720df08b55e55bfe64ee613ca91e54 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Thu, 12 Dec 2024 16:14:23 +0200 Subject: [PATCH 02/15] more notes and docs improvement --- contracts/HydraDelegation/Delegation.sol | 10 ++++++---- contracts/HydraDelegation/HydraDelegation.sol | 5 +++++ .../modules/DelegationPoolLib/DelegationPoolLib.sol | 3 ++- .../modules/VestedDelegation/VestedDelegation.sol | 9 +++++++-- contracts/HydraStaking/HydraStaking.sol | 1 + contracts/VestingManager/VestingManagerFactory.sol | 1 + contracts/common/Liquid/Liquid.sol | 4 ++-- contracts/common/Vesting/VestedPositionLib.sol | 1 + 8 files changed, 25 insertions(+), 9 deletions(-) diff --git a/contracts/HydraDelegation/Delegation.sol b/contracts/HydraDelegation/Delegation.sol index 5e9f2a83..54646f71 100644 --- a/contracts/HydraDelegation/Delegation.sol +++ b/contracts/HydraDelegation/Delegation.sol @@ -216,12 +216,12 @@ contract Delegation is } /** - * @notice Base delegation funds to a staker + * @notice Core logic for delegating funds to a staker * @param staker Address of the validator * @param delegator Address of the delegator * @param amount Amount to delegate */ - function _baseDelegate(address staker, address delegator, uint256 amount) internal virtual { + function _baseDelegate(address staker, address delegator, uint256 amount) internal { if (amount == 0) revert DelegateRequirement({src: "delegate", msg: "DELEGATING_AMOUNT_ZERO"}); DelegationPool storage delegation = delegationPools[staker]; uint256 delegatedAmount = delegation.balanceOf(delegator); @@ -253,6 +253,7 @@ contract Delegation is /** * @notice Undelegates funds from a staker + * @dev overriden in child contracts to extend core undelegate behaviour * @param staker Address of the validator * @param delegator Address of the delegator * @param amount Amount to delegate @@ -262,12 +263,12 @@ contract Delegation is } /** - * @notice Base undelegating funds from a staker + * @notice Core logic for undelegating funds from a staker * @param staker Address of the validator * @param delegator Address of the delegator * @param amount Amount to delegate */ - function _baseUndelegate(address staker, address delegator, uint256 amount) internal virtual { + function _baseUndelegate(address staker, address delegator, uint256 amount) internal { DelegationPool storage delegation = delegationPools[staker]; uint256 delegatedAmount = delegation.balanceOf(delegator); if (amount > delegatedAmount) revert DelegateRequirement({src: "undelegate", msg: "INSUFFICIENT_BALANCE"}); @@ -290,6 +291,7 @@ contract Delegation is /** * @notice Withdraws funds from stakers pool + * @dev overriden in child contracts to extend core withdraw behaviour * @param delegation Delegation pool * @param delegator Address of the delegator * @param amount Amount to withdraw diff --git a/contracts/HydraDelegation/HydraDelegation.sol b/contracts/HydraDelegation/HydraDelegation.sol index 75726e6b..4078f1cb 100644 --- a/contracts/HydraDelegation/HydraDelegation.sol +++ b/contracts/HydraDelegation/HydraDelegation.sol @@ -83,7 +83,12 @@ contract HydraDelegation is IHydraDelegation, System, Delegation, LiquidDelegati */ function _distributeTokens(address staker, address account, uint256 amount) internal virtual override { VestingPosition memory position = vestedDelegationPositions[staker][msg.sender]; + // This check works because if position has already been opened, the restrictions on delegateWithVesting() + // will prevent entering this check again if (_isOpeningPosition(position)) { + // No previous deby or delegation balance is possible, because + // @audit it is possible the user to has previously minted tokens, again from vested delegation, + // but they can be for one week vesting and here he will end with more liquid tokens than for one week vesting uint256 debt = _calculatePositionDebt(amount, position.duration); liquidityDebts[account] -= debt.toInt256Safe(); // Add negative debt amount -= debt; diff --git a/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol b/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol index 576ffcb5..5d226669 100644 --- a/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol +++ b/contracts/HydraDelegation/modules/DelegationPoolLib/DelegationPoolLib.sol @@ -276,7 +276,8 @@ library DelegationPoolLib { // _______________ Private functions _______________ /** - * @notice Saves the RPS for the given staker for the epoch + * @notice Saves the RPS for the given staker's delegation pool + * @dev must be called when new reward is distributed (at the end of every epoch in our case) * @param pool the DelegationPool to save the RPS for * @param rewardPerShare Amount of tokens to be withdrawn * @param epochNumber Epoch number diff --git a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol index ece467cd..f428c4b0 100644 --- a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol +++ b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol @@ -57,6 +57,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve // solhint-disable-next-line func-name-mixedcase function __VestedDelegation_init_unchained() internal onlyInitializing { + // @note check if this is needed as a check to prevent DDOS, otherwise remove it balanceChangeThreshold = 32; } @@ -222,7 +223,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve */ function swapVestedPositionStaker(address oldStaker, address newStaker) external onlyManager { VestingPosition memory oldPosition = vestedDelegationPositions[oldStaker][msg.sender]; - // ensure that the old position is active in order to continue the swap + // ensure that the old position is active if (!oldPosition.isActive()) { revert DelegateRequirement({src: "vesting", msg: "OLD_POSITION_INACTIVE"}); } @@ -386,6 +387,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve // _______________ Internal functions _______________ + // @audit this is not needed /** * @inheritdoc Delegation */ @@ -402,7 +404,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve address delegator, uint256 amount ) internal virtual override { - // If it is a vested delegation, withdraw by keeping the change in the delegation pool params + // If it is a vested delegation, deposit by keeping the change in the delegation pool params // so vested rewards claiming is possible if (vestedDelegationPositions[staker][delegator].isInVestingCycle()) { return delegation.deposit(delegator, amount, hydraChainContract.getCurrentEpochId()); @@ -422,6 +424,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve ) internal virtual override { // If it is an vested delegation, withdraw by keeping the change in the delegation pool params // so vested rewards claiming is possible + // @audit isn't fine to check if the position is active here instead of if it is in vesting cycle? if (vestedDelegationPositions[staker][delegator].isInVestingCycle()) { return delegation.withdraw(delegator, amount, hydraChainContract.getCurrentEpochId()); } @@ -429,6 +432,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve super._withdrawDelegation(staker, delegation, delegator, amount); } + // @audit this function is not used anywhere /** * @inheritdoc Delegation */ @@ -518,6 +522,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve } // If the given RPS is for future time - it is wrong, so revert + if (rpsData.timestamp > alreadyMatured) { revert DelegateRequirement({src: "_verifyRewardsMatured", msg: "WRONG_RPS"}); } diff --git a/contracts/HydraStaking/HydraStaking.sol b/contracts/HydraStaking/HydraStaking.sol index e59b5caa..5a041628 100644 --- a/contracts/HydraStaking/HydraStaking.sol +++ b/contracts/HydraStaking/HydraStaking.sol @@ -242,6 +242,7 @@ contract HydraStaking is currentStake -= amount; // We want all previously distributed tokens from normal stake() to be collected, // because for vested positions we distribute decreased amount of liquid tokens + // @audit what if previously distributed tokens are not from normal stake? (Discuss with Sami) _collectTokens(staker, currentStake); amount += currentStake; } diff --git a/contracts/VestingManager/VestingManagerFactory.sol b/contracts/VestingManager/VestingManagerFactory.sol index f76cf38e..a0bcc20b 100644 --- a/contracts/VestingManager/VestingManagerFactory.sol +++ b/contracts/VestingManager/VestingManagerFactory.sol @@ -52,6 +52,7 @@ contract VestingManagerFactory is IVestingManagerFactory, System, Initializable * @inheritdoc IVestingManagerFactory */ function isVestingManager(address account) external view returns (bool) { + // @note ensure address is different than zero only if vesting manager exists return vestingManagerOwner[account] != address(0); } diff --git a/contracts/common/Liquid/Liquid.sol b/contracts/common/Liquid/Liquid.sol index 2163957f..4140b7e2 100644 --- a/contracts/common/Liquid/Liquid.sol +++ b/contracts/common/Liquid/Liquid.sol @@ -54,12 +54,12 @@ abstract contract Liquid is ILiquid, Initializable { // _______________ Internal functions _______________ - function _collectTokens(address account, uint256 amount) internal virtual { + function _collectTokens(address account, uint256 amount) internal { // User needs to provide their liquid tokens debt as well int256 liquidDebt = liquidityDebts[account]; int256 amountInt = amount.toInt256Safe(); int256 amountAfterDebt = amountInt + liquidDebt; - // if negative debt is bigger than or equal to the amount, we get the whole amount from the debt + // if a negative debt covers the whole amount, no need to burn anything if (amountAfterDebt < 1) { liquidityDebts[account] -= amountInt; // @audit unused variable diff --git a/contracts/common/Vesting/VestedPositionLib.sol b/contracts/common/Vesting/VestedPositionLib.sol index f180e1f1..ef2e6c73 100644 --- a/contracts/common/Vesting/VestedPositionLib.sol +++ b/contracts/common/Vesting/VestedPositionLib.sol @@ -25,6 +25,7 @@ library VestedPositionLib { return vestingEnd <= block.timestamp && block.timestamp < matureEnd; } + // @audit it is said that it returns true if not all rewards from the latest active position are matured yet but doesnt make such check /** * @notice Returns true if the staker/delegator is an active vesting position or not all rewards from the latest active position are matured yet * @param position Vesting position From 449dbb668a24fbf1811ab189eeaf80181d89828d Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Thu, 12 Dec 2024 21:25:23 +0200 Subject: [PATCH 03/15] minor improvements --- contracts/APRCalculator/APRCalculator.sol | 1 + contracts/LiquidityToken/LiquidityToken.sol | 2 +- contracts/PriceOracle/libs/SortedPriceList.sol | 2 ++ contracts/VestingManager/VestingManagerFactory.sol | 3 +-- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/APRCalculator/APRCalculator.sol b/contracts/APRCalculator/APRCalculator.sol index 0591e240..9327fb95 100644 --- a/contracts/APRCalculator/APRCalculator.sol +++ b/contracts/APRCalculator/APRCalculator.sol @@ -99,6 +99,7 @@ contract APRCalculator is IAPRCalculator, MacroFactor, RSIndex { * @notice Initializes vesting bonus for each week. */ function _initializeVestingBonus() private { + // @audit use simplified syntax here vestingBonus[0] = 6; vestingBonus[1] = 16; vestingBonus[2] = 30; diff --git a/contracts/LiquidityToken/LiquidityToken.sol b/contracts/LiquidityToken/LiquidityToken.sol index 2c96ab19..213e31e1 100644 --- a/contracts/LiquidityToken/LiquidityToken.sol +++ b/contracts/LiquidityToken/LiquidityToken.sol @@ -12,7 +12,7 @@ import {ILiquidityToken} from "./ILiquidityToken.sol"; * @title LiquidityToken * @dev This contract represents the liquid token for the Hydra staking mechanism. */ -contract LiquidityToken is ILiquidityToken, System, ERC20Upgradeable, ERC20PermitUpgradeable, Governed { +contract LiquidityToken is ILiquidityToken, System, ERC20PermitUpgradeable, Governed { /// @notice The role identifier for address(es) that have permission to mint and burn the token. bytes32 public constant SUPPLY_CONTROLLER_ROLE = keccak256("SUPPLY_CONTROLLER_ROLE"); diff --git a/contracts/PriceOracle/libs/SortedPriceList.sol b/contracts/PriceOracle/libs/SortedPriceList.sol index d9cf6d9b..5d516ec8 100644 --- a/contracts/PriceOracle/libs/SortedPriceList.sol +++ b/contracts/PriceOracle/libs/SortedPriceList.sol @@ -26,6 +26,8 @@ library SortedPriceList { address current = self.head; // If the new price is smaller or equal to the head, insert at the head (or if the list is empty) + // @audit current == address(0) must be the dirst check + // @note why newNode.next = current in case current == address(0)) if (self.nodes[current].price >= price || current == address(0)) { newNode.next = current; self.head = validator; diff --git a/contracts/VestingManager/VestingManagerFactory.sol b/contracts/VestingManager/VestingManagerFactory.sol index a0bcc20b..4e922dcd 100644 --- a/contracts/VestingManager/VestingManagerFactory.sol +++ b/contracts/VestingManager/VestingManagerFactory.sol @@ -40,7 +40,7 @@ contract VestingManagerFactory is IVestingManagerFactory, System, Initializable BeaconProxy manager = new BeaconProxy( address(beacon), - abi.encodeWithSelector(VestingManager(address(0)).initialize.selector, msg.sender) + abi.encodeWithSelector(VestingManager.initialize.selector, msg.sender) ); _storeVestingManagerData(address(manager), msg.sender); @@ -52,7 +52,6 @@ contract VestingManagerFactory is IVestingManagerFactory, System, Initializable * @inheritdoc IVestingManagerFactory */ function isVestingManager(address account) external view returns (bool) { - // @note ensure address is different than zero only if vesting manager exists return vestingManagerOwner[account] != address(0); } From 8cccd7e64a58bb6a76aa99ff141b2053790ffb4e Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 07:59:38 +0200 Subject: [PATCH 04/15] fixes and improvements --- contracts/APRCalculator/APRCalculator.sol | 107 +++++++++--------- contracts/HydraDelegation/HydraDelegation.sol | 4 +- .../VestedDelegation/VestedDelegation.sol | 16 --- contracts/HydraStaking/HydraStaking.sol | 36 ++++-- contracts/HydraStaking/Staking.sol | 14 --- .../DelegatedStaking/DelegatedStaking.sol | 1 - .../modules/VestedStaking/IVestedStaking.sol | 4 +- .../modules/VestedStaking/VestedStaking.sol | 6 +- .../PriceOracle/libs/SortedPriceList.sol | 3 +- contracts/common/Liquid/Liquid.sol | 2 - docs/HydraChain/HydraChain.md | 4 +- .../HydraChain/modules/Inspector/Inspector.md | 6 +- .../ValidatorManager/ValidatorManager.md | 2 +- docs/HydraStaking/HydraStaking.md | 6 +- docs/HydraStaking/IHydraStaking.md | 4 +- .../modules/VestedStaking/IVestedStaking.md | 4 +- .../modules/VestedStaking/VestedStaking.md | 4 +- test/HydraStaking/VestedStaking.test.ts | 4 +- 18 files changed, 102 insertions(+), 125 deletions(-) diff --git a/contracts/APRCalculator/APRCalculator.sol b/contracts/APRCalculator/APRCalculator.sol index 9327fb95..d8094d09 100644 --- a/contracts/APRCalculator/APRCalculator.sol +++ b/contracts/APRCalculator/APRCalculator.sol @@ -99,59 +99,60 @@ contract APRCalculator is IAPRCalculator, MacroFactor, RSIndex { * @notice Initializes vesting bonus for each week. */ function _initializeVestingBonus() private { - // @audit use simplified syntax here - vestingBonus[0] = 6; - vestingBonus[1] = 16; - vestingBonus[2] = 30; - vestingBonus[3] = 46; - vestingBonus[4] = 65; - vestingBonus[5] = 85; - vestingBonus[6] = 108; - vestingBonus[7] = 131; - vestingBonus[8] = 157; - vestingBonus[9] = 184; - vestingBonus[10] = 212; - vestingBonus[11] = 241; - vestingBonus[12] = 272; - vestingBonus[13] = 304; - vestingBonus[14] = 338; - vestingBonus[15] = 372; - vestingBonus[16] = 407; - vestingBonus[17] = 444; - vestingBonus[18] = 481; - vestingBonus[19] = 520; - vestingBonus[20] = 559; - vestingBonus[21] = 599; - vestingBonus[22] = 641; - vestingBonus[23] = 683; - vestingBonus[24] = 726; - vestingBonus[25] = 770; - vestingBonus[26] = 815; - vestingBonus[27] = 861; - vestingBonus[28] = 907; - vestingBonus[29] = 955; - vestingBonus[30] = 1003; - vestingBonus[31] = 1052; - vestingBonus[32] = 1101; - vestingBonus[33] = 1152; - vestingBonus[34] = 1203; - vestingBonus[35] = 1255; - vestingBonus[36] = 1307; - vestingBonus[37] = 1361; - vestingBonus[38] = 1415; - vestingBonus[39] = 1470; - vestingBonus[40] = 1525; - vestingBonus[41] = 1581; - vestingBonus[42] = 1638; - vestingBonus[43] = 1696; - vestingBonus[44] = 1754; - vestingBonus[45] = 1812; - vestingBonus[46] = 1872; - vestingBonus[47] = 1932; - vestingBonus[48] = 1993; - vestingBonus[49] = 2054; - vestingBonus[50] = 2116; - vestingBonus[51] = 2178; + vestingBonus = [ + 6, + 16, + 30, + 46, + 65, + 85, + 108, + 131, + 157, + 184, + 212, + 241, + 272, + 304, + 338, + 372, + 407, + 444, + 481, + 520, + 559, + 599, + 641, + 683, + 726, + 770, + 815, + 861, + 907, + 955, + 1003, + 1052, + 1101, + 1152, + 1203, + 1255, + 1307, + 1361, + 1415, + 1470, + 1525, + 1581, + 1638, + 1696, + 1754, + 1812, + 1872, + 1932, + 1993, + 2054, + 2116, + 2178 + ]; } // slither-disable-next-line unused-state,naming-convention diff --git a/contracts/HydraDelegation/HydraDelegation.sol b/contracts/HydraDelegation/HydraDelegation.sol index 4078f1cb..33d521a2 100644 --- a/contracts/HydraDelegation/HydraDelegation.sol +++ b/contracts/HydraDelegation/HydraDelegation.sol @@ -60,7 +60,7 @@ contract HydraDelegation is IHydraDelegation, System, Delegation, LiquidDelegati address staker, address delegator, uint256 amount - ) internal virtual override(Delegation, LiquidDelegation, VestedDelegation) { + ) internal virtual override(Delegation, LiquidDelegation) { super._delegate(staker, delegator, amount); } @@ -71,7 +71,7 @@ contract HydraDelegation is IHydraDelegation, System, Delegation, LiquidDelegati address staker, address delegator, uint256 amount - ) internal virtual override(Delegation, LiquidDelegation, VestedDelegation) { + ) internal virtual override(Delegation, LiquidDelegation) { super._undelegate(staker, delegator, amount); } diff --git a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol index f428c4b0..c9167ad8 100644 --- a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol +++ b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol @@ -387,14 +387,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve // _______________ Internal functions _______________ - // @audit this is not needed - /** - * @inheritdoc Delegation - */ - function _delegate(address staker, address delegator, uint256 amount) internal virtual override { - super._delegate(staker, delegator, amount); - } - /** * @inheritdoc Delegation */ @@ -432,14 +424,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve super._withdrawDelegation(staker, delegation, delegator, amount); } - // @audit this function is not used anywhere - /** - * @inheritdoc Delegation - */ - function _undelegate(address staker, address delegator, uint256 amount) internal virtual override { - super._undelegate(staker, delegator, amount); - } - /** * @notice Checks if the position has no reward conditions * @param position The vesting position diff --git a/contracts/HydraStaking/HydraStaking.sol b/contracts/HydraStaking/HydraStaking.sol index 5a041628..2bb71930 100644 --- a/contracts/HydraStaking/HydraStaking.sol +++ b/contracts/HydraStaking/HydraStaking.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.17; +import {Unauthorized} from "../common/Errors.sol"; import {System} from "../common/System/System.sol"; import {SafeMathUint} from "./../common/libs/SafeMathUint.sol"; import {VestingPosition} from "../common/Vesting/IVesting.sol"; @@ -104,7 +105,6 @@ contract HydraStaking is /** * @inheritdoc IHydraStaking */ - // @audit ensure user can't stake unstake and delegations as well, so that balances changed is not invoked function temporaryEjectValidator(address account) external onlyHydraChain { emit BalanceChanged(account, 0); } @@ -134,10 +134,20 @@ contract HydraStaking is // _______________ Internal functions _______________ + /** + * @notice Check if the ban is initiated for the given account + * @param account The address of the account + */ + function _isBanInitiated(address account) internal view returns (bool) { + return hydraChainContract.banIsInitiated(account); + } + /** * @inheritdoc Staking */ function _stake(address account, uint256 amount) internal override(Staking, LiquidStaking, StateSyncStaking) { + if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); + if (stakeOf(account) == 0) { hydraChainContract.activateValidator(account); } @@ -156,6 +166,8 @@ contract HydraStaking is override(Staking, VestedStaking, StateSyncStaking, LiquidStaking) returns (uint256 stakeLeft, uint256 withdrawAmount) { + if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); + (stakeLeft, withdrawAmount) = super._unstake(account, amount); if (stakeLeft == 0) { hydraChainContract.deactivateValidator(account); @@ -180,7 +192,6 @@ contract HydraStaking is } } - // @audit this function can be removed and just using the main _unstake func; beforePenalizeStakeHook must be added to fix liquidity _unstake problem function _executeUnstake( address staker, uint256 unstakeAmount @@ -282,14 +293,13 @@ contract HydraStaking is */ function _distributeReward( uint256 epochId, - Uptime calldata uptime, + Uptime memory uptime, uint256 fullRewardIndex, uint256 totalSupply, uint256 totalBlocks ) private returns (uint256 reward) { if (uptime.signedBlocks > totalBlocks) { - // @audit if a bug arrises in the node, this revert ca stop the whole distribution process. Better in such case set the signedBlocks to totalBlocks - revert DistributeRewardFailed("SIGNED_BLOCKS_EXCEEDS_TOTAL"); + uptime.signedBlocks = totalBlocks; } uint256 currentStake = stakeOf(uptime.validator); @@ -303,15 +313,16 @@ contract HydraStaking is stakerRewardIndex ); - // @audit isn't better check if stakerShares is 0? Same for delegation - _distributeStakingReward(uptime.validator, stakerShares); - _distributeDelegationRewards(uptime.validator, delegatorShares, epochId); - - // Keep history record of the staker rewards to be used on maturing vesting reward claim if (stakerShares != 0) { + _distributeStakingReward(uptime.validator, stakerShares); + // Keep history record of the staker rewards to be used on maturing vesting reward claim _saveStakerRewardData(uptime.validator, epochId); } + if (delegatorShares != 0) { + _distributeDelegationRewards(uptime.validator, delegatorShares, epochId); + } + return stakerRewardIndex; } @@ -326,9 +337,10 @@ contract HydraStaking is uint256 delegatedBalance, uint256 totalReward ) private pure returns (uint256, uint256) { - // @audit what happens if both stakedBalance and delegatedBalance are 0? the reward will be lost. Is that okay? - if (stakedBalance == 0) return (0, totalReward); + // first check if delegated balance is zero + // otherwise if both staked and delegated are zero = reward will be lost if (delegatedBalance == 0) return (totalReward, 0); + if (stakedBalance == 0) return (0, totalReward); uint256 stakerReward = (totalReward * stakedBalance) / (stakedBalance + delegatedBalance); uint256 delegatorReward = totalReward - stakerReward; diff --git a/contracts/HydraStaking/Staking.sol b/contracts/HydraStaking/Staking.sol index 7916a4e0..aa885508 100644 --- a/contracts/HydraStaking/Staking.sol +++ b/contracts/HydraStaking/Staking.sol @@ -110,9 +110,6 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn * @param amount The amount to stake */ function _stake(address account, uint256 amount) internal virtual { - // @audit move check to HydraStaking._stake - if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); - uint256 currentBalance = stakeOf(account); if (amount + currentBalance < minStake) revert StakeRequirement({src: "stake", msg: "STAKE_TOO_LOW"}); @@ -132,9 +129,6 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn address account, uint256 amount ) internal virtual returns (uint256 stakeLeft, uint256 withdrawAmount) { - // @audit move check to HydraStaking._unstake - if (_isBanInitiated(account)) revert Unauthorized("BAN_INITIATED"); - uint256 accountStake = stakeOf(account); if (amount > accountStake) revert StakeRequirement({src: "unstake", msg: "INSUFFICIENT_BALANCE"}); @@ -178,14 +172,6 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn emit StakingRewardsClaimed(staker, rewards); } - /** - * @notice Check if the ban is initiated for the given account - * @param account The address of the account - */ - function _isBanInitiated(address account) internal view returns (bool) { - return hydraChainContract.banIsInitiated(account); - } - // _______________ Private functions _______________ /** diff --git a/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol b/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol index efacad56..31fe5c61 100644 --- a/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol +++ b/contracts/HydraStaking/modules/DelegatedStaking/DelegatedStaking.sol @@ -84,7 +84,6 @@ abstract contract DelegatedStaking is IDelegatedStaking, Staking { * @return The total amount of delegation */ function _totalDelegation() internal view returns (uint256) { - // @audit check of total delegation is properly changed on ban procedures and similar return delegationContract.totalDelegation(); } diff --git a/contracts/HydraStaking/modules/VestedStaking/IVestedStaking.sol b/contracts/HydraStaking/modules/VestedStaking/IVestedStaking.sol index 53b12fd2..0921fabd 100644 --- a/contracts/HydraStaking/modules/VestedStaking/IVestedStaking.sol +++ b/contracts/HydraStaking/modules/VestedStaking/IVestedStaking.sol @@ -57,10 +57,10 @@ interface IVestedStaking is IVesting, IStaking { * @param staker The address of the staker * @param amount The amount that is going to be unstaked * @return penalty for the staker - * @return reward of the staker + * @return rewardToBurn of the staker */ function calcVestedStakingPositionPenalty( address staker, uint256 amount - ) external view returns (uint256 penalty, uint256 reward); + ) external view returns (uint256 penalty, uint256 rewardToBurn); } diff --git a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol index 5a2c5476..24c5adba 100644 --- a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol +++ b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol @@ -134,17 +134,15 @@ abstract contract VestedStaking is IVestedStaking, Vesting, Staking { /** * @inheritdoc IVestedStaking */ - // @audit this function returns wrong data for reward function calcVestedStakingPositionPenalty( address staker, uint256 amount - ) external view returns (uint256 penalty, uint256 reward) { - reward = stakingRewards[staker].total - stakingRewards[staker].taken; + ) external view returns (uint256 penalty, uint256 rewardToBurn) { VestingPosition memory position = vestedStakingPositions[staker]; if (position.isActive()) { penalty = _calcPenalty(position, amount); // if active position, reward is burned - reward = 0; + rewardToBurn = stakingRewards[staker].total - stakingRewards[staker].taken; } } diff --git a/contracts/PriceOracle/libs/SortedPriceList.sol b/contracts/PriceOracle/libs/SortedPriceList.sol index 5d516ec8..a4770ee9 100644 --- a/contracts/PriceOracle/libs/SortedPriceList.sol +++ b/contracts/PriceOracle/libs/SortedPriceList.sol @@ -26,9 +26,8 @@ library SortedPriceList { address current = self.head; // If the new price is smaller or equal to the head, insert at the head (or if the list is empty) - // @audit current == address(0) must be the dirst check // @note why newNode.next = current in case current == address(0)) - if (self.nodes[current].price >= price || current == address(0)) { + if (current == address(0) || self.nodes[current].price >= price) { newNode.next = current; self.head = validator; self.nodes[validator] = newNode; diff --git a/contracts/common/Liquid/Liquid.sol b/contracts/common/Liquid/Liquid.sol index 4140b7e2..1edd592d 100644 --- a/contracts/common/Liquid/Liquid.sol +++ b/contracts/common/Liquid/Liquid.sol @@ -62,8 +62,6 @@ abstract contract Liquid is ILiquid, Initializable { // if a negative debt covers the whole amount, no need to burn anything if (amountAfterDebt < 1) { liquidityDebts[account] -= amountInt; - // @audit unused variable - amount = 0; return; } diff --git a/docs/HydraChain/HydraChain.md b/docs/HydraChain/HydraChain.md index 06d78323..71be90e2 100644 --- a/docs/HydraChain/HydraChain.md +++ b/docs/HydraChain/HydraChain.md @@ -223,7 +223,7 @@ Returns if a ban process is initiated for a given validator function banThreshold() external view returns (uint256) ``` -Validator inactiveness (in seconds) threshold that needs to be passed to ban a validator +Validator inactiveness (in seconds) threshold. A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. @@ -729,7 +729,7 @@ Method used to initiate a ban for validator, if the initiate ban threshold is re function initiateBanThreshold() external view returns (uint256) ``` -Validator inactiveness (in blocks) threshold that needs to be passed to initiate ban for a validator +Validator inactiveness (in blocks) threshold that needs to be reached or passed to initiate ban for a validator diff --git a/docs/HydraChain/modules/Inspector/Inspector.md b/docs/HydraChain/modules/Inspector/Inspector.md index 7aea17a9..c416136d 100644 --- a/docs/HydraChain/modules/Inspector/Inspector.md +++ b/docs/HydraChain/modules/Inspector/Inspector.md @@ -223,7 +223,7 @@ Returns if a ban process is initiated for a given validator function banThreshold() external view returns (uint256) ``` -Validator inactiveness (in seconds) threshold that needs to be passed to ban a validator +Validator inactiveness (in seconds) threshold. A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. @@ -479,9 +479,9 @@ Method used to initiate a ban for validator, if the initiate ban threshold is re function initiateBanThreshold() external view returns (uint256) ``` -Validator inactiveness (in blocks) threshold that needs to be passed to initiate ban for a validator - +Validator inactiveness (in blocks) threshold that needs to be reached or passed to initiate ban for a validator +*must be always bigger than the epoch length (better bigger than at least 4 epochs), otherwise all validators can be banned* #### Returns diff --git a/docs/HydraChain/modules/ValidatorManager/ValidatorManager.md b/docs/HydraChain/modules/ValidatorManager/ValidatorManager.md index 22e0bcab..96544970 100644 --- a/docs/HydraChain/modules/ValidatorManager/ValidatorManager.md +++ b/docs/HydraChain/modules/ValidatorManager/ValidatorManager.md @@ -694,7 +694,7 @@ function validatorsParticipation(address) external view returns (uint256) Mapping that keeps the last time when a validator has participated in the consensus -*Keep in mind that the validator will initially be set active when stake, but it will be able to participate in the next epoch. So, the validator will have less blocks to participate before getting eligible for ban.* +*Updated on epoch-ending blocks onlyKeep in mind that the validator will initially be set active when stake, but it will be able to participate in the next epoch. So, the validator will have less blocks to participate before getting eligible for ban.* #### Parameters diff --git a/docs/HydraStaking/HydraStaking.md b/docs/HydraStaking/HydraStaking.md index af8def1c..9ef30bbc 100644 --- a/docs/HydraStaking/HydraStaking.md +++ b/docs/HydraStaking/HydraStaking.md @@ -183,7 +183,7 @@ function aprCalculatorContract() external view returns (contract IAPRCalculator) ### calcVestedStakingPositionPenalty ```solidity -function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 reward) +function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 rewardToBurn) ``` Returns the penalty and reward that will be burned, if vested stake position is active @@ -202,7 +202,7 @@ Returns the penalty and reward that will be burned, if vested stake position is | Name | Type | Description | |---|---|---| | penalty | uint256 | for the staker | -| reward | uint256 | of the staker | +| rewardToBurn | uint256 | of the staker | ### calculateOwedLiquidTokens @@ -528,7 +528,7 @@ Register withdrawal of the penalized funds function lastDistribution() external view returns (uint256) ``` - +last rewards distribution timestamp diff --git a/docs/HydraStaking/IHydraStaking.md b/docs/HydraStaking/IHydraStaking.md index 19b86e8e..69ebf825 100644 --- a/docs/HydraStaking/IHydraStaking.md +++ b/docs/HydraStaking/IHydraStaking.md @@ -13,7 +13,7 @@ ### calcVestedStakingPositionPenalty ```solidity -function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 reward) +function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 rewardToBurn) ``` Returns the penalty and reward that will be burned, if vested stake position is active @@ -32,7 +32,7 @@ Returns the penalty and reward that will be burned, if vested stake position is | Name | Type | Description | |---|---|---| | penalty | uint256 | for the staker | -| reward | uint256 | of the staker | +| rewardToBurn | uint256 | of the staker | ### calculateOwedLiquidTokens diff --git a/docs/HydraStaking/modules/VestedStaking/IVestedStaking.md b/docs/HydraStaking/modules/VestedStaking/IVestedStaking.md index 34b882aa..b4d3690a 100644 --- a/docs/HydraStaking/modules/VestedStaking/IVestedStaking.md +++ b/docs/HydraStaking/modules/VestedStaking/IVestedStaking.md @@ -13,7 +13,7 @@ ### calcVestedStakingPositionPenalty ```solidity -function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 reward) +function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 rewardToBurn) ``` Returns the penalty and reward that will be burned, if vested stake position is active @@ -32,7 +32,7 @@ Returns the penalty and reward that will be burned, if vested stake position is | Name | Type | Description | |---|---|---| | penalty | uint256 | for the staker | -| reward | uint256 | of the staker | +| rewardToBurn | uint256 | of the staker | ### calculatePositionClaimableReward diff --git a/docs/HydraStaking/modules/VestedStaking/VestedStaking.md b/docs/HydraStaking/modules/VestedStaking/VestedStaking.md index fd7916f8..46f3234e 100644 --- a/docs/HydraStaking/modules/VestedStaking/VestedStaking.md +++ b/docs/HydraStaking/modules/VestedStaking/VestedStaking.md @@ -81,7 +81,7 @@ function aprCalculatorContract() external view returns (contract IAPRCalculator) ### calcVestedStakingPositionPenalty ```solidity -function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 reward) +function calcVestedStakingPositionPenalty(address staker, uint256 amount) external view returns (uint256 penalty, uint256 rewardToBurn) ``` Returns the penalty and reward that will be burned, if vested stake position is active @@ -100,7 +100,7 @@ Returns the penalty and reward that will be burned, if vested stake position is | Name | Type | Description | |---|---|---| | penalty | uint256 | for the staker | -| reward | uint256 | of the staker | +| rewardToBurn | uint256 | of the staker | ### calculatePositionClaimableReward diff --git a/test/HydraStaking/VestedStaking.test.ts b/test/HydraStaking/VestedStaking.test.ts index 26630808..40e471bd 100644 --- a/test/HydraStaking/VestedStaking.test.ts +++ b/test/HydraStaking/VestedStaking.test.ts @@ -228,7 +228,7 @@ export function RunVestedStakingTests(): void { const position = await hydraStaking.vestedStakingPositions(this.staker.address); const latestTimestamp = hre.ethers.BigNumber.from(await time.latest()); // get the penalty and reward from the contract - const { penalty, reward } = await hydraStaking.calcVestedStakingPositionPenalty( + const { penalty, rewardToBurn } = await hydraStaking.calcVestedStakingPositionPenalty( this.staker.address, this.minStake ); @@ -238,7 +238,7 @@ export function RunVestedStakingTests(): void { expect(penalty, "penalty").to.be.gt(0); expect(penalty, "penalty = calculatedPenalty").to.be.equal(calculatedPenalty); - expect(reward, "reward").to.be.equal(0); // if active position, reward is burned + expect(rewardToBurn, "rewardToBurn").to.be.equal(0); // if active position, reward is burned }); it("should decrease staking position and apply slashing penalty", async function () { From 0a19e64bc39a1a2a3e04dc25220127f779b9ce5f Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 10:25:09 +0200 Subject: [PATCH 05/15] fixes and improvements --- contracts/HydraDelegation/HydraDelegation.sol | 13 +++++++---- .../VestedDelegation/VestedDelegation.sol | 17 +------------- contracts/HydraStaking/HydraStaking.sol | 12 ++++------ contracts/HydraStaking/Staking.sol | 4 +++- .../modules/VestedStaking/VestedStaking.sol | 3 +-- docs/HydraDelegation/HydraDelegation.md | 17 -------------- .../VestedDelegation/VestedDelegation.md | 17 -------------- test/HydraStaking/VestedStaking.test.ts | 23 +++++++++++++++---- 8 files changed, 38 insertions(+), 68 deletions(-) diff --git a/contracts/HydraDelegation/HydraDelegation.sol b/contracts/HydraDelegation/HydraDelegation.sol index 33d521a2..5059b098 100644 --- a/contracts/HydraDelegation/HydraDelegation.sol +++ b/contracts/HydraDelegation/HydraDelegation.sol @@ -39,7 +39,6 @@ contract HydraDelegation is IHydraDelegation, System, Delegation, LiquidDelegati __Liquid_init(liquidToken); __Vesting_init_unchained(); __VestingManagerFactoryConnector_init(vestingManagerFactoryAddr); - __VestedDelegation_init_unchained(); } // _______________ External functions _______________ @@ -86,9 +85,15 @@ contract HydraDelegation is IHydraDelegation, System, Delegation, LiquidDelegati // This check works because if position has already been opened, the restrictions on delegateWithVesting() // will prevent entering this check again if (_isOpeningPosition(position)) { - // No previous deby or delegation balance is possible, because - // @audit it is possible the user to has previously minted tokens, again from vested delegation, - // but they can be for one week vesting and here he will end with more liquid tokens than for one week vesting + uint256 previousDelegation = delegationOf(staker, account) - amount; + if (previousDelegation != 0) { + // We want all previously distributed tokens to be collected, + // because the new position vesting period can be different from the previous one + // meaning the tokens to distribute amount will be different + _collectTokens(staker, previousDelegation); + amount += previousDelegation; + } + uint256 debt = _calculatePositionDebt(amount, position.duration); liquidityDebts[account] -= debt.toInt256Safe(); // Add negative debt amount -= debt; diff --git a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol index c9167ad8..a4d97321 100644 --- a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol +++ b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol @@ -16,12 +16,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve using DelegationPoolLib for DelegationPool; using VestedPositionLib for VestingPosition; - /** - * @notice The threshold for the maximum number of allowed balance changes - * @dev We are using this to restrict unlimited changes of the balance (delegationPoolParamsHistory) - */ - uint256 public balanceChangeThreshold; - /** * @notice The vesting positions for every delegator * @dev Staker => Delegator => VestingPosition @@ -52,13 +46,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve ); __Vesting_init(governance, aprCalculatorAddr); __VestingManagerFactoryConnector_init(vestingManagerFactoryAddr); - __VestedDelegation_init_unchained(); - } - - // solhint-disable-next-line func-name-mixedcase - function __VestedDelegation_init_unchained() internal onlyInitializing { - // @note check if this is needed as a check to prevent DDOS, otherwise remove it - balanceChangeThreshold = 32; } // _______________ Modifiers _______________ @@ -416,8 +403,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve ) internal virtual override { // If it is an vested delegation, withdraw by keeping the change in the delegation pool params // so vested rewards claiming is possible - // @audit isn't fine to check if the position is active here instead of if it is in vesting cycle? - if (vestedDelegationPositions[staker][delegator].isInVestingCycle()) { + if (vestedDelegationPositions[staker][delegator].isActive()) { return delegation.withdraw(delegator, amount, hydraChainContract.getCurrentEpochId()); } @@ -506,7 +492,6 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve } // If the given RPS is for future time - it is wrong, so revert - if (rpsData.timestamp > alreadyMatured) { revert DelegateRequirement({src: "_verifyRewardsMatured", msg: "WRONG_RPS"}); } diff --git a/contracts/HydraStaking/HydraStaking.sol b/contracts/HydraStaking/HydraStaking.sol index 2bb71930..cfbbf5b3 100644 --- a/contracts/HydraStaking/HydraStaking.sol +++ b/contracts/HydraStaking/HydraStaking.sol @@ -248,14 +248,12 @@ contract HydraStaking is // This check works because if position has already been opened, the restrictions on stake() and stakeWithVesting() // will prevent entering the check again if (_isOpeningPosition(position)) { - uint256 currentStake = stakeOf(staker); - if (currentStake != amount) { - currentStake -= amount; - // We want all previously distributed tokens from normal stake() to be collected, + uint256 previousStake = stakeOf(staker) - amount; + if (previousStake != 0) { + // We want all previously distributed tokens to be collected, // because for vested positions we distribute decreased amount of liquid tokens - // @audit what if previously distributed tokens are not from normal stake? (Discuss with Sami) - _collectTokens(staker, currentStake); - amount += currentStake; + _collectTokens(staker, previousStake); + amount += previousStake; } uint256 debt = _calculatePositionDebt(amount, position.duration); diff --git a/contracts/HydraStaking/Staking.sol b/contracts/HydraStaking/Staking.sol index aa885508..c3bf56b5 100644 --- a/contracts/HydraStaking/Staking.sol +++ b/contracts/HydraStaking/Staking.sol @@ -84,7 +84,7 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn * @inheritdoc IStaking */ function claimStakingRewards() public { - rewardWalletContract.distributeReward(msg.sender, _claimStakingRewards(msg.sender)); + _claimStakingRewards(msg.sender); } /** @@ -169,6 +169,8 @@ contract Staking is IStaking, Withdrawal, HydraChainConnector, APRCalculatorConn stakingRewards[staker].taken += rewards; + rewardWalletContract.distributeReward(staker, rewards); + emit StakingRewardsClaimed(staker, rewards); } diff --git a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol index 24c5adba..72b9e700 100644 --- a/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol +++ b/contracts/HydraStaking/modules/VestedStaking/VestedStaking.sol @@ -46,8 +46,7 @@ abstract contract VestedStaking is IVestedStaking, Vesting, Staking { if (vestedStakingPositions[msg.sender].isInVestingCycle()) { revert StakeRequirement({src: "stakeWithVesting", msg: "ALREADY_IN_VESTING_CYCLE"}); } - // Claim the rewards before opening a new position, to avoid locking them during vesting cycle - // @audit this claim function doesn't actually claim the rewards. Bug!!! + if (unclaimedRewards(msg.sender) != 0) _claimStakingRewards(msg.sender); // Clear the staking rewards history diff --git a/docs/HydraDelegation/HydraDelegation.md b/docs/HydraDelegation/HydraDelegation.md index a2e0f7fc..0161aed4 100644 --- a/docs/HydraDelegation/HydraDelegation.md +++ b/docs/HydraDelegation/HydraDelegation.md @@ -208,23 +208,6 @@ function aprCalculatorContract() external view returns (contract IAPRCalculator) |---|---|---| | _0 | contract IAPRCalculator | undefined | -### balanceChangeThreshold - -```solidity -function balanceChangeThreshold() external view returns (uint256) -``` - -The threshold for the maximum number of allowed balance changes - - - - -#### Returns - -| Name | Type | Description | -|---|---|---| -| _0 | uint256 | undefined | - ### calculateOwedLiquidTokens ```solidity diff --git a/docs/HydraDelegation/modules/VestedDelegation/VestedDelegation.md b/docs/HydraDelegation/modules/VestedDelegation/VestedDelegation.md index b5fd5b69..8bde5f3b 100644 --- a/docs/HydraDelegation/modules/VestedDelegation/VestedDelegation.md +++ b/docs/HydraDelegation/modules/VestedDelegation/VestedDelegation.md @@ -106,23 +106,6 @@ function aprCalculatorContract() external view returns (contract IAPRCalculator) |---|---|---| | _0 | contract IAPRCalculator | undefined | -### balanceChangeThreshold - -```solidity -function balanceChangeThreshold() external view returns (uint256) -``` - -The threshold for the maximum number of allowed balance changes - -*We are using this to restrict unlimited changes of the balance (delegationPoolParamsHistory)* - - -#### Returns - -| Name | Type | Description | -|---|---|---| -| _0 | uint256 | undefined | - ### calculatePositionClaimableReward ```solidity diff --git a/test/HydraStaking/VestedStaking.test.ts b/test/HydraStaking/VestedStaking.test.ts index 40e471bd..c93dcd4f 100644 --- a/test/HydraStaking/VestedStaking.test.ts +++ b/test/HydraStaking/VestedStaking.test.ts @@ -60,16 +60,31 @@ export function RunVestedStakingTests(): void { expect(await hydraStaking.stakeOf(this.staker.address), "stake").to.be.equal(this.minStake); }); - it("should open vested position with the old stake base and adjust token balance", async function () { - const { hydraStaking, liquidToken } = await loadFixture(this.fixtures.stakedValidatorsStateFixture); + it.only("should open vested position with the old stake base and adjust token balance", async function () { + const { hydraStaking, systemHydraChain, liquidToken } = await loadFixture( + this.fixtures.stakedValidatorsStateFixture + ); + + await commitEpochs( + systemHydraChain, + hydraStaking, + [this.signers.validators[0], this.signers.validators[1], this.staker], + 5, // number of epochs to commit + this.epochSize + ); const validator = this.signers.validators[0]; await hydraStaking.connect(validator).stake({ value: this.minStake.mul(20) }); const tokenBalance = await liquidToken.balanceOf(validator.address); + const rewardBeforeOpeningVestedPosition = await hydraStaking.unclaimedRewards(validator.address); + expect(rewardBeforeOpeningVestedPosition).to.be.gt(0); - await hydraStaking.connect(validator).stakeWithVesting(52, { value: this.minStake }); + // stake with vesting must distribute rewards from the previous stake + await expect( + hydraStaking.connect(validator).stakeWithVesting(52, { value: this.minStake }) + ).to.changeEtherBalance(validator.address, this.minStake.sub(rewardBeforeOpeningVestedPosition).mul(-1)); const currentStake = await hydraStaking.stakeOf(validator.address); const expectedLiquidTokens = calcLiquidTokensToDistributeOnVesting(52, currentStake); @@ -238,7 +253,7 @@ export function RunVestedStakingTests(): void { expect(penalty, "penalty").to.be.gt(0); expect(penalty, "penalty = calculatedPenalty").to.be.equal(calculatedPenalty); - expect(rewardToBurn, "rewardToBurn").to.be.equal(0); // if active position, reward is burned + expect(rewardToBurn, "rewardToBurn").to.be.gt(0); // if active position, reward is burned }); it("should decrease staking position and apply slashing penalty", async function () { From 35ba1e4fdb2f52f6c14bb5e5d5e1f609eb3b90ce Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 10:53:02 +0200 Subject: [PATCH 06/15] fixes --- .../ValidatorManager/ValidatorManager.sol | 18 ------------------ .../modules/ValidatorsData/ValidatorsData.sol | 2 -- contracts/PriceOracle/libs/SortedPriceList.sol | 1 - 3 files changed, 21 deletions(-) diff --git a/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol b/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol index 13596228..8bc60f6f 100644 --- a/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol +++ b/contracts/HydraChain/modules/ValidatorManager/ValidatorManager.sol @@ -73,24 +73,6 @@ abstract contract ValidatorManager is } } - // _______________ Modifiers _______________ - - // @audit unused modifier - modifier onlyActiveValidator(address validator) { - if (validators[validator].status != ValidatorStatus.Active) revert Unauthorized("INACTIVE_VALIDATOR"); - _; - } - - // @audit unused modifier - /// @notice Modifier to check if the validator is registered or active - modifier onlyValidator(address validator) { - if ( - validators[validator].status != ValidatorStatus.Registered && - validators[validator].status != ValidatorStatus.Active - ) revert Unauthorized("INVALID_VALIDATOR"); - _; - } - // _______________ External functions _______________ /** diff --git a/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol b/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol index 9a95c663..d42a9de8 100644 --- a/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol +++ b/contracts/HydraChain/modules/ValidatorsData/ValidatorsData.sol @@ -26,11 +26,9 @@ abstract contract ValidatorsData is IValidatorsData, System, Initializable { return; } - // @audt you were able to use a single int256 variable to store the changes in the power uint256 totalNewPower = 0; uint256 totalOldPower = 0; for (uint256 i = 0; i < arrLength; i++) { - // @audit naming is confusing totalNewPower += validatorsPower[i].votingPower; totalOldPower += validatorPower[validatorsPower[i].validator]; validatorPower[validatorsPower[i].validator] = validatorsPower[i].votingPower; diff --git a/contracts/PriceOracle/libs/SortedPriceList.sol b/contracts/PriceOracle/libs/SortedPriceList.sol index a4770ee9..a4a14627 100644 --- a/contracts/PriceOracle/libs/SortedPriceList.sol +++ b/contracts/PriceOracle/libs/SortedPriceList.sol @@ -26,7 +26,6 @@ library SortedPriceList { address current = self.head; // If the new price is smaller or equal to the head, insert at the head (or if the list is empty) - // @note why newNode.next = current in case current == address(0)) if (current == address(0) || self.nodes[current].price >= price) { newNode.next = current; self.head = validator; From 1e9947b14b67e7d60a330faae4e691497475e000 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 11:02:58 +0200 Subject: [PATCH 07/15] modify check --- .../modules/VestedDelegation/VestedDelegation.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol index a4d97321..9ad0be3b 100644 --- a/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol +++ b/contracts/HydraDelegation/modules/VestedDelegation/VestedDelegation.sol @@ -385,7 +385,7 @@ abstract contract VestedDelegation is IVestedDelegation, Vesting, Delegation, Ve ) internal virtual override { // If it is a vested delegation, deposit by keeping the change in the delegation pool params // so vested rewards claiming is possible - if (vestedDelegationPositions[staker][delegator].isInVestingCycle()) { + if (vestedDelegationPositions[staker][delegator].isActive()) { return delegation.deposit(delegator, amount, hydraChainContract.getCurrentEpochId()); } From 2a6559f84257515285d95dcf2cd20634eeba039f Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 11:11:37 +0200 Subject: [PATCH 08/15] fix comment and enable tests --- contracts/common/Vesting/VestedPositionLib.sol | 3 +-- test/HydraStaking/VestedStaking.test.ts | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/common/Vesting/VestedPositionLib.sol b/contracts/common/Vesting/VestedPositionLib.sol index ef2e6c73..895db040 100644 --- a/contracts/common/Vesting/VestedPositionLib.sol +++ b/contracts/common/Vesting/VestedPositionLib.sol @@ -25,9 +25,8 @@ library VestedPositionLib { return vestingEnd <= block.timestamp && block.timestamp < matureEnd; } - // @audit it is said that it returns true if not all rewards from the latest active position are matured yet but doesnt make such check /** - * @notice Returns true if the staker/delegator is an active vesting position or not all rewards from the latest active position are matured yet + * @notice Returns true if the staker/delegator is an active vesting position or the max maturing period is not yet reached * @param position Vesting position * @return bool Returns true if the position is in vesting cycle */ diff --git a/test/HydraStaking/VestedStaking.test.ts b/test/HydraStaking/VestedStaking.test.ts index c93dcd4f..658d8594 100644 --- a/test/HydraStaking/VestedStaking.test.ts +++ b/test/HydraStaking/VestedStaking.test.ts @@ -60,7 +60,7 @@ export function RunVestedStakingTests(): void { expect(await hydraStaking.stakeOf(this.staker.address), "stake").to.be.equal(this.minStake); }); - it.only("should open vested position with the old stake base and adjust token balance", async function () { + it("should open vested position with the old stake base and adjust token balance", async function () { const { hydraStaking, systemHydraChain, liquidToken } = await loadFixture( this.fixtures.stakedValidatorsStateFixture ); From f252a158506543f273b1c23692f629c0d66a81c7 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 17:29:11 +0200 Subject: [PATCH 09/15] Fix audit comments left --- contracts/HydraChain/HydraChain.sol | 6 ------ .../modules/DaoIncentive/DaoIncentive.sol | 15 +++++++-------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/contracts/HydraChain/HydraChain.sol b/contracts/HydraChain/HydraChain.sol index 0dffb3ff..39c03cad 100644 --- a/contracts/HydraChain/HydraChain.sol +++ b/contracts/HydraChain/HydraChain.sol @@ -13,17 +13,14 @@ import {IHydraChain} from "./IHydraChain.sol"; import {Epoch} from "./IHydraChain.sol"; contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { - // @note unsafe arrays are used in the code using ArraysUpgradeable for uint256[]; uint256 public currentEpochId; - // @note this is unsafe array /// @notice Array with epoch ending blocks uint256[] public epochEndBlocks; /// @notice Epoch data linked with the epoch id mapping(uint256 => Epoch) public epochs; - // @audit isn't this the same as the epochEndBlocks? mapping(uint256 => uint256) internal _commitBlockNumbers; // _______________ Initializer _______________ @@ -71,7 +68,6 @@ contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { if (epoch.startBlock >= epoch.endBlock) { revert CommitEpochFailed("NO_BLOCKS_COMMITTED"); } - // @audit this is going to be a problem when slashing is added if ((epoch.endBlock - epoch.startBlock + 1) % epochSize != 0) { revert CommitEpochFailed("EPOCH_MUST_BE_DIVISIBLE_BY_EPOCH_SIZE"); } @@ -149,10 +145,8 @@ contract HydraChain is IHydraChain, Inspector, DaoIncentive, ValidatorsData { uint256 validatorParticipation = validatorsParticipation[validator]; // check if the validator is active and the last participation is less than the threshold if ( - // @audit here we check if the validator is active but this doesn't mean it is actually part of the active set (check if this is breakable) validators[validator].status == ValidatorStatus.Active && lastCommittedEndBlock > validatorParticipation && - // @audit at block 1 what is the value of validatorParticipation? lastCommittedEndBlock is 0 in the first 500 blocks lastCommittedEndBlock - validatorParticipation >= initiateBanThreshold ) { return true; diff --git a/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol b/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol index 88276a3b..6e5bb64c 100644 --- a/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol +++ b/contracts/HydraChain/modules/DaoIncentive/DaoIncentive.sol @@ -39,9 +39,9 @@ abstract contract DaoIncentive is IDaoIncentive, System, Initializable, RewardWa * @inheritdoc IDaoIncentive */ function distributeDAOIncentive() external onlySystemCall { - // @audit put all multiplications before division to avoid rounding errors - uint256 reward = (((hydraStakingContract.totalBalance() * 200) / 10000) * - (block.timestamp - lastDistribution)) / 365 days; + uint256 reward = ( + ((hydraStakingContract.totalBalance() * 200 * (block.timestamp - lastDistribution)) / (10000 * 365 days)) + ); lastDistribution = block.timestamp; vaultDistribution += reward; @@ -52,16 +52,15 @@ abstract contract DaoIncentive is IDaoIncentive, System, Initializable, RewardWa * @inheritdoc IDaoIncentive */ function claimVaultFunds() external { - // @audit reward can be renamed to incentive or something - uint256 reward = vaultDistribution; - if (reward == 0) { + uint256 incentive = vaultDistribution; + if (incentive == 0) { revert NoVaultFundsToClaim(); } vaultDistribution = 0; - rewardWalletContract.distributeReward(daoIncentiveVaultContract, reward); + rewardWalletContract.distributeReward(daoIncentiveVaultContract, incentive); - emit VaultFunded(reward); + emit VaultFunded(incentive); } // slither-disable-next-line unused-state,naming-convention From c978ccd22cca7b99f01ee6a25a232977fe0f5bcb Mon Sep 17 00:00:00 2001 From: Rosen Santev <77731162+R-Santev@users.noreply.github.com> Date: Fri, 13 Dec 2024 17:36:49 +0200 Subject: [PATCH 10/15] Update docs/HydraChain/HydraChain.md Co-authored-by: Vitomir Pavlov --- docs/HydraChain/HydraChain.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/HydraChain/HydraChain.md b/docs/HydraChain/HydraChain.md index 71be90e2..ce00d551 100644 --- a/docs/HydraChain/HydraChain.md +++ b/docs/HydraChain/HydraChain.md @@ -223,7 +223,7 @@ Returns if a ban process is initiated for a given validator function banThreshold() external view returns (uint256) ``` -Validator inactiveness (in seconds) threshold. A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. +Threshold for validator inactiveness (in seconds). A validator can be banned if it remains in the ban-initiated state for a duration equal to or exceeding this threshold. From eb2377fcd69b4bad5b2d573acff535ae314f1316 Mon Sep 17 00:00:00 2001 From: Rosen Santev <77731162+R-Santev@users.noreply.github.com> Date: Fri, 13 Dec 2024 17:37:22 +0200 Subject: [PATCH 11/15] Update docs/HydraChain/HydraChain.md Co-authored-by: Vitomir Pavlov --- docs/HydraChain/HydraChain.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/HydraChain/HydraChain.md b/docs/HydraChain/HydraChain.md index ce00d551..dd4be789 100644 --- a/docs/HydraChain/HydraChain.md +++ b/docs/HydraChain/HydraChain.md @@ -729,7 +729,7 @@ Method used to initiate a ban for validator, if the initiate ban threshold is re function initiateBanThreshold() external view returns (uint256) ``` -Validator inactiveness (in blocks) threshold that needs to be reached or passed to initiate ban for a validator +Threshold for validator inactiveness (in blocks). A ban can be initiated for a validator if this threshold is reached or exceeded. From 4a33cb3d81bc156833e91c8ff0194cb9c4d58a5d Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 17:39:39 +0200 Subject: [PATCH 12/15] Minor improvements --- contracts/HydraDelegation/Delegation.sol | 4 ++-- contracts/LiquidityToken/LiquidityToken.sol | 1 - contracts/PriceOracle/libs/SortedPriceList.sol | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/HydraDelegation/Delegation.sol b/contracts/HydraDelegation/Delegation.sol index 54646f71..388a4b5e 100644 --- a/contracts/HydraDelegation/Delegation.sol +++ b/contracts/HydraDelegation/Delegation.sol @@ -253,7 +253,7 @@ contract Delegation is /** * @notice Undelegates funds from a staker - * @dev overriden in child contracts to extend core undelegate behaviour + * @dev overridden in child contracts to extend core undelegate behaviour * @param staker Address of the validator * @param delegator Address of the delegator * @param amount Amount to delegate @@ -291,7 +291,7 @@ contract Delegation is /** * @notice Withdraws funds from stakers pool - * @dev overriden in child contracts to extend core withdraw behaviour + * @dev overridden in child contracts to extend core withdraw behaviour * @param delegation Delegation pool * @param delegator Address of the delegator * @param amount Amount to withdraw diff --git a/contracts/LiquidityToken/LiquidityToken.sol b/contracts/LiquidityToken/LiquidityToken.sol index 213e31e1..de9e567a 100644 --- a/contracts/LiquidityToken/LiquidityToken.sol +++ b/contracts/LiquidityToken/LiquidityToken.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.17; -import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; import {ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol"; import {System} from "../common/System/System.sol"; diff --git a/contracts/PriceOracle/libs/SortedPriceList.sol b/contracts/PriceOracle/libs/SortedPriceList.sol index a4a14627..d9cf6d9b 100644 --- a/contracts/PriceOracle/libs/SortedPriceList.sol +++ b/contracts/PriceOracle/libs/SortedPriceList.sol @@ -26,7 +26,7 @@ library SortedPriceList { address current = self.head; // If the new price is smaller or equal to the head, insert at the head (or if the list is empty) - if (current == address(0) || self.nodes[current].price >= price) { + if (self.nodes[current].price >= price || current == address(0)) { newNode.next = current; self.head = validator; self.nodes[validator] = newNode; From 06367f8d5bdd845f37e9497bbcfb6c688385db12 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 17:59:54 +0200 Subject: [PATCH 13/15] fix failing test --- test/HydraDelegation/VestedDelegation.test.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/HydraDelegation/VestedDelegation.test.ts b/test/HydraDelegation/VestedDelegation.test.ts index 020e6659..c2bb831c 100644 --- a/test/HydraDelegation/VestedDelegation.test.ts +++ b/test/HydraDelegation/VestedDelegation.test.ts @@ -232,6 +232,12 @@ export function RunVestedDelegationTests(): void { this.fixtures.vestedDelegationFixture ); + const delegatedAmount = await hydraDelegation.delegationOf(this.delegatedValidators[0], vestManager.address); + const amountToCut = delegatedAmount.div(2); + const amount = await hydraDelegation.calculateOwedLiquidTokens(vestManager.address, amountToCut); + await liquidToken.connect(vestManagerOwner).approve(vestManager.address, amount); + await vestManager.connect(vestManagerOwner).cutVestedDelegatePosition(this.delegatedValidators[0], amountToCut); + // go in the maturing phase await time.increase(WEEK * 16); @@ -246,22 +252,16 @@ export function RunVestedDelegationTests(): void { ); expect(mature, "mature").to.be.true; - const delegatedAmount = await hydraDelegation.delegationOf(this.delegatedValidators[0], vestManager.address); - const amountToDelegate = this.minDelegation.mul(2); - const amount = await hydraDelegation.calculateOwedLiquidTokens(vestManager.address, delegatedAmount.div(2)); - await liquidToken.connect(vestManagerOwner).approve(vestManager.address, amount); - await vestManager - .connect(vestManagerOwner) - .cutVestedDelegatePosition(this.delegatedValidators[0], delegatedAmount.div(2)); - // check if the balance change is made const epochNum = await hydraChain.getCurrentEpochId(); - const isBalanceMadeChange = await hydraDelegation.isBalanceChangeMade( + const isBalanceChangeMade = await hydraDelegation.isBalanceChangeMade( this.delegatedValidators[0], vestManager.address, epochNum ); - expect(isBalanceMadeChange, "isBalanceMadeChange").to.be.true; + expect(isBalanceChangeMade, "isBalanceChangeMade").to.be.true; + + const amountToDelegate = this.minDelegation.mul(2); const tx = await vestManager.openVestedDelegatePosition(this.delegatedValidators[0], vestingDuration, { value: amountToDelegate, }); @@ -272,7 +272,7 @@ export function RunVestedDelegationTests(): void { vestManager.address, this.delegatedValidators[0], vestingDuration, - amountToDelegate.add(delegatedAmount.div(2)) + amountToDelegate.add(delegatedAmount.sub(amountToCut)) ); expect(await hydraDelegation.delegationOf(this.delegatedValidators[0], vestManager.address)).to.be.equal( From c20dde470bf76947dd9c4ac5ba27c96e8286d0c3 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 18:30:12 +0200 Subject: [PATCH 14/15] fix docs --- contracts/HydraChain/modules/Inspector/Inspector.sol | 8 ++++---- docs/HydraChain/modules/Inspector/Inspector.md | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/HydraChain/modules/Inspector/Inspector.sol b/contracts/HydraChain/modules/Inspector/Inspector.sol index 346e869c..fcc6a10d 100644 --- a/contracts/HydraChain/modules/Inspector/Inspector.sol +++ b/contracts/HydraChain/modules/Inspector/Inspector.sol @@ -12,13 +12,13 @@ abstract contract Inspector is IInspector, ValidatorManager { uint256 public validatorPenalty; /// @notice The reward for the person who reports a validator that have to be banned uint256 public reporterReward; - /// @notice Validator inactiveness (in blocks) threshold - /// that needs to be reached or passed to initiate ban for a validator + /// @notice Threshold for validator inactiveness (in blocks). + /// A ban can be initiated for a validator if this threshold is reached or exceeded. /// @dev must be always bigger than the epoch length (better bigger than at least 4 epochs), /// otherwise all validators can be banned uint256 public initiateBanThreshold; - /// @notice Validator inactiveness (in seconds) threshold. - /// A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. + /// @notice Threshold for validator inactiveness (in seconds). A validator can be banned + /// if it remains in the ban-initiated state for a duration equal to or exceeding this threshold. uint256 public banThreshold; /// @notice Mapping of the validators that bans has been initiated for (validator => timestamp) mapping(address => uint256) public bansInitiated; diff --git a/docs/HydraChain/modules/Inspector/Inspector.md b/docs/HydraChain/modules/Inspector/Inspector.md index c416136d..b073f2eb 100644 --- a/docs/HydraChain/modules/Inspector/Inspector.md +++ b/docs/HydraChain/modules/Inspector/Inspector.md @@ -223,7 +223,7 @@ Returns if a ban process is initiated for a given validator function banThreshold() external view returns (uint256) ``` -Validator inactiveness (in seconds) threshold. A validator can be banned if in "ban initiated" state for a duration equal to or exceeding this threshold. +Threshold for validator inactiveness (in seconds). A validator can be banned if it remains in the ban-initiated state for a duration equal to or exceeding this threshold. @@ -479,7 +479,7 @@ Method used to initiate a ban for validator, if the initiate ban threshold is re function initiateBanThreshold() external view returns (uint256) ``` -Validator inactiveness (in blocks) threshold that needs to be reached or passed to initiate ban for a validator +Threshold for validator inactiveness (in blocks). A ban can be initiated for a validator if this threshold is reached or exceeded. *must be always bigger than the epoch length (better bigger than at least 4 epochs), otherwise all validators can be banned* From bada1ef29751f82ee95198ca3fd164d23cf426e1 Mon Sep 17 00:00:00 2001 From: Rosen Santev Date: Fri, 13 Dec 2024 18:41:45 +0200 Subject: [PATCH 15/15] extend staking rewards claiming test --- test/HydraStaking/HydraStaking.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/HydraStaking/HydraStaking.test.ts b/test/HydraStaking/HydraStaking.test.ts index a5519f6b..2f089837 100644 --- a/test/HydraStaking/HydraStaking.test.ts +++ b/test/HydraStaking/HydraStaking.test.ts @@ -297,13 +297,13 @@ export function RunHydraStakingTests(): void { const reward = await hydraStaking.unclaimedRewards(rewardingValidator.address); - await expect( - hydraStaking.connect(rewardingValidator).stakeWithVesting(3, { - value: this.minStake, - }) - ) - .to.emit(hydraStaking, "StakingRewardsClaimed") - .withArgs(rewardingValidator.address, reward); + const tx = hydraStaking.connect(rewardingValidator).stakeWithVesting(3, { + value: this.minStake, + }); + + await expect(tx).to.emit(hydraStaking, "StakingRewardsClaimed").withArgs(rewardingValidator.address, reward); + + await expect(tx).to.changeEtherBalance(rewardingValidator.address, this.minStake.sub(reward).mul(-1)); expect(await hydraStaking.unclaimedRewards(rewardingValidator.address)).to.be.equal(0); });