Staking Updates

IDynamicStaking.Claim names its third parameter penaltyAmount, implying that the value represents a penalty. However, when a user claims, the contract emits the actual reward amount: emit Claim(sender, stakeIdPart, actualClaimedAmount, claimFee).

Off-chain indexers or dashboards that rely on ABI field names for semantics may misinterpret claimed rewards as penalties, leading to incorrect accounting or misleading alerts.

Rename the parameter and its associated comment in the interface (and any downstream consumers) to accurately reflect actual behavior.

Key parameters such as minStakeAmount, stake time bounds, withdrawal delays, penalty caps, the whitelist toggle, and the fee manager were changed from public to internal without introducing replacement getter functions. Additionally, the previous helper getMinimumStakeAmount was removed in this PR.

As a result, users and off-chain tools have no way to determine minimum stake amounts, lock periods, or whether proofless staking is allowed without directly inspecting raw storage.

Re-expose these parameters through public getters or dedicated view functions so that external systems retain visibility into the staking configuration.

DefaultCompositeMultiplierHook.getMultiplier composes four per-hook multipliers as (A * B / MULTIPLIER_SCALER) * C / MULTIPLIER_SCALER * D / MULTIPLIER_SCALER with MULTIPLIER_SCALER = 1e4. Each / MULTIPLIER_SCALER is integer division, so the result is floored three times in sequence. The mathematically exact value is (A × B × C × D) / MULTIPLIER_SCALER³ with a single rounding at the end; the current form drops fractional units after every intermediate step, so the composite multiplier is systematically ≤ the exact value. That biases virtual stake and reward share slightly down versus a single final division.

Compute in one shot after promoting to uint256, for example uint256(A) * B * C * D / (uint256(MULTIPLIER_SCALER) ** 3) then cast to uint32 after a bounds check. With each hook capped at 5 * MULTIPLIER_SCALER (Common.sol), the product fits in uint256 without overflow.

The multiExecute function is public and performs external calls to arbitrary addresses with arbitrary calldata. As a result, users can execute a wide range of external calls through the MultiExecute contract.

This contract must never hold token balances and should not be granted approvals, as its entire token balance (excluding ETH) could be transferred and any approvals could be exploited via multiExecute.

Document this behavior and clearly warn users not to fund or approve the contract unless operations are executed atomically, as all funds held by the contract can be swept.

A state variable in NftMultiplierHook contains a typo, defined as nftMulitplier instead of nftMultiplier.

Additionally, the UnexpetedEthReceived, AlreadyTermintated, and AlreadyForecefullyTermintated errors contain typos.

Update the variable names and errors with typos and all functions that reference them.

The WithDefundSupport._defund function uses send to transfer native ETH. It is best practice to use the low-level call{value: amount}("") pattern instead, as send forwards only 2,300 gas

Consider using low-level call instead of send.

The BatchDeploy contract contains two functions - deployContract() and multiExecute() - and it inherits from the Multicall contract. The Multicall.multicall() function lets users batch different function calls of the same contract by utilizing delegatecall. In the case of the current contract, users can call multiExecute() and deployContract() in the same call. However, multicall() is non-payable, so it only works for multiExecute() calls that don't perform native token payments.

Functionally, there are no benefits of using multicall, because multiExecute() can be used to call deployContract() as well and there is no caller specific logic there. The only upside of using multicall is that it returns bytes[] return data, while multiExecute() ignores it.

Consider whether inheriting Multicall is necessary in this contract. If you decide to remove it, you can also modify multiExecute() to return the internal calls return data, just like currently multicall() does.

deployContract loops over deploymentCodes and calls Create2.deploy(0, salt, deploymentCodes[i]) with the same salt for every index. CREATE2 resolves the contract address from deployer, salt, and the init code hash. If two entries share identical bytecode, they share the same init code hash, so the second deployment targets an address that already has code from the first iteration. OpenZeppelin’s Create2.deploy then reverts. Because the whole call runs in one transaction, the entire batch reverts.

Derive a per-entry salt so identical bytecode still maps to distinct addresses

IMultiplierHook.getMultiplier and IPenaltyHook.getPenaltyAmount are declared as external with returns (...) but without the view modifier.

Add view to both interface signatures so implementations must be read-only at compile time.

The following functions change critical protocol state without emitting any event: modifyCampaign, setMerkleRoot, setEmergencyMode, setAllowStakingWithoutProof, setFeeParams, modifyMaximumRewardGenerationRate, modifyMinStakeAmount, withdrawAllPenaltyAmount, withdrawEthBalance, defundContractBalance (DynamicStaking.sol); setStakingContractAddress (PostWithdrawalHookBase.sol); setPenaltyRange (StakeTimeRangePenaltyHook.sol); setStakeAmountRange, setStakeTimeRange, setNftMultiplier (DefaultCompositeMultiplierHook.sol).

Add events to all state-changing admin functions

DynamicStaking.unstake() enforces two independent penalty checks:

1. require(penaltyAmount == expectedPenaltyAmount) user slippage protection.
2. require(penaltyAmount * PENALTY_SCALER * 100 / actualUnstaked <= maxPenaltyPercentage) immutable protocol cap set at deployment.

These serve different purposes: the first protects the user against a rogue admin front-running the penalty hook; the second is a hard on-chain guarantee to all stakers that penalties will never exceed a fixed percentage regardless of hook configuration.

However, maxPenaltyPercentage is immutable it cannot be changed after deployment. If it is set lower than the maximum penalty the hook can legitimately return (e.g. maxPenaltyPercentage = 10% but hook charges 50% for early exits), the second check will always revert for early unstakers. They cannot exit early under any circumstances and must wait until unstakableFrom when penalty is 0.

Document clearly that maxPenaltyPercentage must be >= the maximum penalty percentage the configured hook can ever return.

setRole() calls _setRoleAdmin(role, role) for both FORCE_LIQUIDATE_POSITION_ROLE and DEFUND_CONTRACT_BALANCE_ROLE. Under OpenZeppelin AccessControl, only the role admin can grant or revoke that role, so ADMIN_ROLE cannot revoke a compromised holder. A compromised DEFUND_CONTRACT_BALANCE_ROLE holder can grant the role to additional addresses before anyone can react, then call defundContractBalance() and drain principal, rewards, penalties, and pending entries. There is no override path for ADMIN_ROLE once proliferation starts.

Use _setRoleAdmin(role, ADMIN_ROLE) for both roles so ADMIN_ROLE can revoke holders. For DEFUND_CONTRACT_BALANCE_ROLE especially, prefer a multi-sig rather than an EOA.

DynamicStaking accepts arbitrary multiplier, penalty, and post-withdrawal hook addresses in its constructor and does not enforce that these addresses point to the reference implementations provided in the repository. As a result, the deployer can supply any contracts they control.

A malicious or buggy hook can reject transactions, confiscate tokens, or reroute withdrawals. Users must perform due diligence and trust the pool deployer, as well as the specific hook contracts selected. There is no on-chain registry of audited hooks, nor any restrictions preventing the deployer from setting arbitrary hooks.

Document this behavior clearly and warn users that deployers can freely choose hook contracts.

If only the reference hooks in this repository are intended to be used, maintain a list of approved hook contracts and enforce on-chain restrictions during deployment.

A comment in withdrawToRecipient suggests that a custom post-withdrawal hook can be used to differentiate between claims and unstakes:

// withdrawToRecipient handles claims and stakes without any distinction, so if
// for example claims should always use direct transfer and stakes should use a hook
// then the solution is to disable direct transfer and create a special hook that can differentiate
// between claims and stakes

However, the hook interface does not provide enough information to achieve this. When a caller passes multiple entry types (e.g., [Unstake, Claim]), removePendingEntries processes both types sequentially and sums the results into a single actuallyRemovedAmount. The hook then receives:

• amount: the aggregate withdrawn amount across all types
• pendingEntryTypes: the array of types that were requested, not what was actually consumed

There is no per-type amount breakdown passed to the hook. Given amount = 150 and pendingEntryTypes = [Unstake, Claim], the hook cannot determine whether 100 came from unstakes and 50 from claims, or any other split.

A hook that needs to apply different logic to claims vs unstakes (e.g., different fee structures, routing, or access control) cannot function correctly for mixed withdrawals. The comment's suggested mitigation is incomplete — it relies on callers voluntarily making separate single-type calls, which cannot be enforced on-chain from the hook side.

If this is a desired feature, pass per-type amounts to the hook so it can apply type-specific logic without depending on caller cooperation. Otherwise, remove the comment.

In DynamicStaking.withdrawToRecipient(), when isDirectTransfer = false and failOnZeroWithdrawal = false, the postWithdrawalHook.handlePostWithdrawal() is called unconditionally; even when actuallyRemovedAmount == 0 (for example no pending entries have matured yet).

if (actuallyRemovedAmount != 0) {
    token.safeTransfer(address(postWithdrawalHook), actuallyRemovedAmount);
}
// called regardless of actuallyRemovedAmount
postWithdrawalHook.handlePostWithdrawal(
    token, actuallyRemovedAmount, sender, recipient, stakeIdPart, pendingEntryTypes, extraData
);

Since integrators can deploy their own hook implementations extending PostWithdrawalHookBase, they must be aware that _handlePostWithdrawal can be invoked with amount = 0. A hook implementation that assumes a non-zero amount on every call will behave incorrectly in this case.

Add a prominent notice to PostWithdrawalHookBase and its documentation that _handlePostWithdrawal may be called with amount = 0. Integrators should always guard their logic with an early return or explicit check.

The NftMultiplierHook.getMultiplier() function returns the stored nftMulitplier if the staker owns the NFT. Depending on which collection this hook is used for, users can transfer the same NFT among each other in order to benefit from the stake multiplier.

This behavior of the hook must be taken into consideration when configuring a multiplier and a collection.

There is a comment in DynamicStaking that highlights the possibility of a malicious hook owner DOS-ing user stakes by changing the multiplier.

        // each staking/unstaking(with penalty) in which case the admin can DoS a particular user.
        // This is not a major issue as users can always submit his transaction to a node with private pool.
        // If DoS turns out to be an issue in the future, then an update window feature can be implemented.

The comment suggest that users can use private pools to avoid this, but in reality, the owner of the penalty hook can just set the penalty to an unreasonable value that the user wouldn't agree with, for example 100%, achieving the same DOS effect.

The owner of the PostWithdrawalHookBase can call setStakingContractAddress() and change the staking address to an invalid address - this would DOS withdrawals for all users.

Make sure the users are aware of the risks related to hook owners.

StakeTimeRangePenaltyHook.getPenaltyPercentage() and RangeMultiplierBase.getRangeMultiplier() perform redundant --i on the last line, as the i variable is no longer used after that.

    function getPenaltyPercentage(uint32 value) internal view returns (uint32 penaltyPercentage) {
        uint256 rangeLength = penaltyRange.length;

        uint256 i = 1;
        while (i < rangeLength && penaltyRange[i].remainingTimePercentageLowerBound <= value) {
            ++i;
        }
        penaltyPercentage = penaltyRange[--i].penaltyPercentage;
    }

    function getRangeMultiplier(MultiplierRangeEntry[] storage range, uint256 value)
        internal
        view
        returns (uint32 multiplier)
    {
        uint256 rangeLength = range.length;

        uint256 i = 1;
        while (i < rangeLength && range[i].lowerBound <= value) {
            ++i;
        }
        multiplier = range[--i].multiplier;
    }

Consider using i - 1 instead.

- penaltyPercentage = penaltyRange[--i].penaltyPercentage;
+ penaltyPercentage = penaltyRange[i - 1].penaltyPercentage;

- multiplier = range[--i].multiplier;
+ multiplier = range[i - 1].multiplier;

Previously, there were 2 typos in the following errors:NotYetForecefullyTermintated (forEcefully and terminTated) and AlreadyForecefullyTermintated (forEcefully and terminTated). The latest commit fixed the E typo, but the T typo still remains.

Additionally, the following typos exist in comments, NatSpec documentation, and one named return identifier:

• DynamicStaking.sol#L532 — rouge should be rogue ("a small risk of a rouge admin").
• DynamicStaking.sol#L548 — transfering should be transferring.
• IDynamicStaking.sol#L62 — migth should be might, vaule should be value.
• IDynamicStaking.sol#L143 — migth should be might, vaule should be value.
• FixedStaking.sol#L320 — exectute should be execute.
• FixedStaking.sol#L352 — rouge should be rogue.
• FixedStaking.sol#L460 — preceeding should be preceding.
• IFixedStaking.sol#L96 — named return compoundingPeriodLenght should be compoundingPeriodLength to match the function name.

Fix all of the typos. The compoundingPeriodLenght named return in IFixedStaking.sol is purely cosmetic (named returns in interfaces are not part of the ABI selector), but should be aligned with the function name compoundingPeriodLength for consistency.

In WithDefundSupport._defund(), the native-token branch is selected via tokenParam == NATIVE_TOKEN. Since both operands are contract-typed values, Solidity emits a compiler warning for direct contract comparison and recommends comparing their addresses explicitly. This does not currently change runtime behavior, but it adds avoidable warning noise to builds and makes the sentinel-address intent less explicit than an address(...) comparison.

Compare the sentinel values through address(...) in WithDefundSupport._defund(), for example address(tokenParam) == address(NATIVE_TOKEN). This removes the compiler warning and makes the native-token check explicit and consistent with the surrounding zero-address validation.

The comment in DynamicStaking.unstake() says users only need to verify that the construction-time supplied penalty and multiplier hooks are not malicious as “a one-time check”. That guidance is only accurate if the referenced hooks are immutable and cannot be upgraded or materially reconfigured after deployment. If a hook is upgradeable, owner-controlled, or depends on mutable external state, its behavior can change over time and the trust assumption must be revisited continuously. Leaving the comment as-is may cause integrators and users to underestimate the ongoing trust and monitoring requirements of the configured hooks.

Update the comment to clarify that hook verification is only a one-time check when the hooks are immutable and non-upgradeable. If hooks are upgradeable or admin-configurable, document that users and integrators should treat them as ongoing trust assumptions and monitor them accordingly.

MultiExecute.deployContract() derives each CREATE2 salt as bytes32(uint256(salt) + i) without binding it to the caller. As a result, deterministic deployment addresses are namespaced only by the MultiExecute address, the derived salt, and the init code, not by the initiating user. Different users can therefore target the same deterministic deployment address by reusing the same salt and init code, or by choosing overlapping salt ranges across batches. This means callers cannot safely assume that a predictable address is uniquely tied to their own deployment flow or initiator identity. Any integrator or deployed contract logic that informally treats such an address as being attributable to a specific user, transaction origin, or deployment attempt is relying on a false trust assumption.

Scope the derived salt to the initiator in deployContract(), for example by hashing the caller together with the user-supplied salt and index. This makes deterministic deployment addresses user-specific and avoids collisions or attribution ambiguity across independent callers. If shared salts are intentional, document clearly that deterministic addresses produced by deployContract() are not bound to a particular caller.

MultiExecute exposes deployContract() for deterministic CREATE2 deployments but does not expose a view/helper function to compute the resulting deployment address from the supplied salt, batch index, and init code. While the address can be derived off-chain, the missing getter makes integration harder for contracts and on-chain workflows that need to reason about a future deployment address before it is created. This reduces composability, forces downstream users to reimplement the derivation logic externally, and increases the chance of mismatches if the derivation scheme is replicated incorrectly.

Add a view/helper function that returns the deployment address for a given salt, batch index, and deployment bytecode, and optionally a batch variant for multiple bytecodes. This would make deterministic deployments easier to consume from both off-chain tooling and on-chain integrations without requiring external reimplementation of the address derivation logic.