WilderWorld

Staking & Governance

6.1 Unstaking with exit allows unauthorized rewards claim

In the current implementation, when a user initiates an unstake operation with the exit flag set to true for a token that is still locked, the system behaves as expected: it unstakes the token but does not transfer rewards to the user. However, it fails to properly reset the owedRewardsLocked variable, which holds the pre-computed reward amount for the entire lock duration.

function _unstakeMany(...) internal {
    // ...
    for (i; i < _tokenIds.length;){
        // ...
        if (nftStaker.locked[_tokenIds[i]]) {
            // ...
             if (_getRemainingLockTime(nftStaker.stake) == 0) {
                  // ...
                  nftStaker.stake.owedRewardsLocked = 0; // @audit reset only when claimed
                   rewardsGivenLocked = true;
              }
              // ...
            _unstake(_tokenIds[i]);
            // ...
            }
            // ...
    }
  // ... 
    if (!exit) {
            // Transfer the user's rewards
            _transferAmount(config.rewardsToken, rewards);
            emit Claimed(msg.sender, rewards);
    }
}

As a result, the user can claim these rewards via the claim() function in the case of a partial unstake. Alternatively, the user can make another stake to access these rewards. This allows malicious users to exploit the system by creating stakes with long lock durations, exiting the stake early, and then claiming pre-computed rewards, potentially depleting the contract's funds.

Recommendation

The owedRewardsLocked value should be reset when unstaking locked funds.

6.2 Potential reward manipulation due to incorrect reward multiplier

In the _coreStake(...) function, when a user stakes a new amount with a locking period while their previous lock has not yet expired, the new stake is only locked for the remaining time of the previous lock. However, the rewards for the new stake are still calculated using the previous lock's rewardsMultiplier, which was intended for the entire previous lock duration. This results in the new stake receiving a disproportionately high rewards multiplier based on the full original lock period, even though it is only locked for the remaining time.

function _coreStake(...) internal {
    // ...
    if (block.timestamp > staker.unlockedTimestamp) {
        // ...
     } else {
         staker.owedRewardsLocked += _getStakeRewards(
                    amount,
                    staker.rewardsMultiplier, //@audit multiplier for the previous lock duration
                    _getRemainingLockTime(staker),
                    true);
    }
}

This issue can be exploited by users who stake large amounts near the end of their previous lock period. They can benefit from a higher rewards multiplier based on the full lock duration despite their new stake being locked for a much shorter period.

Recommendation

The rewardsMultiplier should be recomputed to reflect the remaining lock time for the new stake rather than relying on the multiplier from the previous lock.

6.3 Staking parameters change impact past rewards computations

The owner of the staking contracts has the ability to modify configuration values that impact reward calculations, notably:

uint256 rewardsPerPeriod
uint256 periodLength
uint256 minimumRewardsMultiplier
uint256 maximumRewardsMultiplier

However, in the current design, changes to these parameters impact rewards computation for historical stakes and not the newest ones only.

For example, if a user stakes tokens and the owner later changes the reward parameters during the staking period ( updates the rewardsPerPeriod, for example), the system currently recalculates all previous rewards using the new configuration. This leads to a jump in accrued rewards, where rewards from earlier periods are either over- or under-calculated, depending on the direction of the change.

function _getStakeRewards(...) internal view returns (uint256) {
    uint256 divisor = locked ? LOCKED_PRECISION_DIVISOR : PRECISION_DIVISOR;

    return rewardsMultiplier * amount * config.rewardsPerPeriod * timeDuration / config.periodLength / divisor;
}

Recommendation

Ensure that changes to configuration values should only apply to rewards from the time they are updated and should not affect rewards for past periods. Alternatively, all user rewards must be accounted for and stored before performing any parameter update.

6.4 User loses rewards when unstaking partial amounts

When a user unstakes a portion of their tokens, the contract computes rewards based on the withdrawn amount rather than the total amount staked. Refer to the code snippet below, where accrued rewards are being computed using the amount withdrawn.

function _unstake(uint256 amount, bool locked, bool exit) internal {
    // ...
    // most recent timestamp?
    rewards = staker.owedRewards + _getStakeRewards(
        amount, //@audit only rewards for unstaked amount are taken
        1, // Rewards multiplier
        block.timestamp - staker.lastTimestamp,
        false
    );
    if (staker.amountStaked == amount) {
       ...
    } else {
        staker.amountStaked -= amount;
        staker.lastTimestamp = block.timestamp;
    }
    // ...
}

After the partial unstake, the contract updates the staker.lastTimestamp to the current block.timestamp, considering that the entire staking period rewards are accounted for. Consequently, users lose the accrued rewards for the remaining staked amount.

Note that the present issue applies to both the ERC20 and ERC721 staking contracts, including both locked and non-locked tokens.

Recommendation

Use the total staked amount instead of the withdrawn amount when calculating the accrued rewards. For ERC20Staking, the staker.amountStaked can be used, while in ERC721Staking, the number of staked tokens must be accounted for.

6.5 Array length manipulation allows partial exit in _unstakeMany(...)

The staking process is designed to enforce that a user must perform a full exit when unstaking. In the StakingERC721 case, this is enforced by the following check in the _unstakeMany(...) function.

if (exit && _tokenIds.length < nftStaker.stake.amountStakedLocked) {
        revert NotFullExit();
}

However, the function currently allows users to submit arbitrary or invalid token IDs. These invalid IDs are skipped during processing, but they are still counted in the array length. As a result, users can manipulate the array length to bypass the full exit check and perform a partial exit.

if (
    nftStaker.staked[_tokenIds[i]] == false
    || IERC721(config.stakeRepToken).ownerOf(_tokenIds[i]) == address(0)
    || IERC721(config.stakeRepToken).ownerOf(_tokenIds[i]) != msg.sender
) {
    // Either the list of tokenIds contains a non-existent token
    // or it contains a token the owner doesnt own
    unchecked {
        ++i;
    }
    continue;
}

Recommendation

Update the function logic to avoid relying on the array length, as it can be manipulated.

6.6 Automatic re-locking of unlocked tokens and rewards in staking flow

Currently, when an existing user stakes additional tokens in locked mode and the previous lock duration has expired, the system automatically includes the previously unlocked amount as part of the new staked and locked amount.

From the user's perspective, the initially locked tokens are unlocked, and their rewards become claimable. However, if the user stakes additional tokens, the previously unlocked tokens are re-locked for a new period, alongside the new tokens. As a result, the previously accumulated rewards are also locked again. If the user attempts to exit during this new lock period, they risk losing the rewards earned during the previous lock period, even though the original lock was successfully completed.

function _coreStake(
    Staker storage staker,
    uint256 amount,
    uint256 lockDuration
) internal {
    if (lockDuration == 0) {
        // ...
    } else {
        if (block.timestamp > staker.unlockedTimestamp) {
            // The user has never locked or they have and we are past their lock period

            // get the user's owed rewards from period in between `unlockedTimestamp` and present at rate of 1
            uint256 mostRecentTimestamp = _mostRecentTimestamp(staker);

            // this will return 0 if `amountStakedLocked` == 0
            staker.owedRewardsLocked += _getStakeRewards(
                staker.amountStakedLocked,
                1,
                block.timestamp - mostRecentTimestamp,
                false
            );

            // Then we update appropriately
            staker.unlockedTimestamp = block.timestamp + lockDuration;
            staker.rewardsMultiplier = _calcRewardsMultiplier(lockDuration);

            // We precalculate the amount because we know the time frame
            staker.owedRewardsLocked += _getStakeRewards(
                amount,
                staker.rewardsMultiplier,
                lockDuration,
                true
            );
        } else {...}

        staker.lastTimestampLocked = block.timestamp;
        //@audit the cumulative amount is locked 
        staker.amountStakedLocked += amount;
    }
}

Recommendation

Consider allowing users to unstake and claim their locked rewards once the timelock has expired. Alternatively, document the current process clearly to avoid user mistakes that could result in the locking or loss of accrued rewards.

6.7 Stale locked state might lead to reward discrepancies

The nftStaker.locked field is set during staking and stores whether a token Id is locked or not.

During the unstaking process of a locked token, this field is not reset to false.

function _unstakeMany(...) internal {
    // ...
    for (i; i < _tokenIds.length;){
        // ...
        if (nftStaker.locked[_tokenIds[i]]) {
            // ...
            _unstake(_tokenIds[i]);
            --nftStaker.stake.amountStakedLocked;
            nftStaker.staked[_tokenIds[i]] = false; 
            isAction = true;
            }
            // ...
    }
  // ... 
}

As a result, if the same token is restaked without a lock, the stale locked entry remains true. This creates an inconsistency where the token is marked as locked in the system, but no rewards are actually computed for the locked stake.

Recommendation

Ensure that the nftStaker.locked flag is properly reset to false after a token is unstaked.

6.8 Absence of reentrancy protection in staking contracts

The StakingBase contract inherits from OpenZeppelin's ReentrancyGuard to prevent reentrancy vulnerabilities. However, for this protection to work, functions within the StakingBase contract and its derived contracts must be explicitly marked with the nonReentrant modifier.

Currently, the nonReentrant modifier has not been applied to any functions in the StakingBase contract nor in its derived contracts (StakingERC20 and StakingERC721).

Recommendation

Assign the nonReentrant modifier to the necessary staking functions.

6.9 Duplicated functions to receive native tokens

In the current implementation, both the receive() and fallback() functions are defined, but both have empty logic. The receive() function is designed to handle native token transfers without data, while the fallback() function can handle transfers with or without data.

Since both functions serve the same purpose — allowing the contract to receive native tokens — and neither contains any additional logic, only one function is necessary.

Recommendation

Remove the redundant fallback() function.

6.10 Incorrect setup of Multiplier for locked rewards could prevent staking

The locked rewards are calculated based on reward multipliers and the duration for which the tokens are locked. One of the factors for lock rewards calculation is the difference between maximumRewardsMultiplier and minimumRewardsMultiplier configured by the owner of the staking contracts.

function _calcRewardsMultiplier(uint256 lock) internal view returns (uint256) {
    return config.minimumRewardsMultiplier
    + (config.maximumRewardsMultiplier - config.minimumRewardsMultiplier)
    * (lock)
    / config.periodLength;
}

However, the setter functions for these parameters lack validation to ensure that maximumRewardsMultiplier is already greater than minimumRewardsMultiplier. Setting invalid values will prevent users from staking tokens for a locked duration. That is because when the _calcRewardsMultiplier(...) function is called during staking, an underflow exception will occur.

Likewise, the function reverts when the periodLength is set to 0.

Recommendation

Add validations in setMinimumRewardsMultiplier and setMaximumRewardsMultiplier setter functions to enforce that maximumRewardsMultiplier is always greater than minimumRewardsMultiplier.

Additionally, ensure that periodLength is always greater than 0 to avoid potential reverts.

6.11 Incorrect total supply of tokens returned by ZeroVotingERC721

The total supply of tokens in ZeroVotingERC721 contract is tracked using the _totalSupply state variable. The total supply should reflect the number of tokens in circulation after excluding the tokens that were burnt.

The current implementation of ZeroVotingERC721 contract increments the _totalSupply state variable when minting the new tokens through mint(...) and safeMint(...) functions, but it is not decremented when tokens are burnt through burn(...) function.

Therefore, _totalSupply does not correctly reflect the number of tokens currently in circulation.

Recommendation

The _totalSupply state variable should be decremented when a token is burnt to reflect the total number of tokens in circulation correctly.

6.12 Unnecessary payable operator for unstaking functions

The payable operator is unnecessary for unstaking functions in StakingERC20 contract. These functions do not require any native token payment.

function unstake(uint256 amount, bool exit) external payable override {
    _unstake(amount, false, exit);
}

function unstakeLocked(uint256 amount, bool exit) external payable override {
    _unstake(amount, true, exit);
}

Recommendation

Remove the payable operator from the above functions.

6.13 Unstaking non-locked tokens with exit cancels rewards

The staking contract allows users to stake tokens in both locked and unlocked modes. In locked mode, users earn higher rewards, but the staked tokens cannot be withdrawn before the end of the lock period. If a user attempts to exit from a locked stake, they forfeit all rewards. However, in unlocked mode, users have the flexibility to withdraw their stake at any time without forfeiting their rewards.

The current implementation of the StakingERC20 contract incorrectly implements this logic. When unstaking, if the user sets the exit flag to true, rewards are lost even in non-locked mode.

function _unstake(uint256 amount, bool locked, bool exit) internal {
    // ...
    if (locked) {...} 
    else { // @audit non-locked mode
        if (amount > staker.amountStaked) {
            revert UnstakeMoreThanStake();
        }

        if (exit) {
            rewards = 0; //@audit rewards are removed  
            staker.owedRewards;
        } else {...}
        // ...
    }
    // ...
}

This issue is also present in the unstaking of ERC721 tokens.

Recommendation

Revise the logic to stop forfeiting rewards in unlocked mode. The exit flag should be applicable only to the locked mode of staking.

Update from Nethermind Security

The Wilderworld team confirmed during a call that this behavior is intentional. The exit flow has been restructured as a separate function, allowing users to withdraw only their initially staked tokens. This new functionality is particularly useful in cases where the contract is unable to provide reward tokens.

6.14 Unused ERC20Permit implementation in ZeroVotingERC20

The ZeroVotingERC20 contract inherits from the ERC20Permit contract but restricts token transfers through the _update() function, which is intended to prevent transfers of tokens. The ERC20Permit implementation introduces the permit(...) function that allows users to approve token transfers using signed messages. However, since the ZeroVotingERC20 contract explicitly disables token transfers, the permit(...) function becomes non-functional and, therefore, inheriting from the ERC20Permit contract is redundant.

function _update(
    address from,
    address to,
    uint256 value
) internal override(ERC20, ERC20Votes) {
    if (from != address(0) && to != address(0)) {
    revert NonTransferrableToken();
    }
    super._update(from, to, value);
}

Recommendation

Remove the unused inheritance of ERC20Permit from the ZeroVotingERC20 contract.

6.15 Usage of transfer() to move native tokens

In order to transfer native tokens, the StakingBase contract currently uses the transfer function. This is not a recommended function to move native tokens due to a fixed gas limit of 2300. As the gas cost is subject to change as part of infra upgrades.

Additionally, when the recipient is a smart contract like a multi-sig wallet, 2300 gas might not cover the gas required to complete the transfer, leading to a revert.

Recommendation

Use a low-level .call function to move native tokens. Additionally, the reentrancy guard should be implemented in different functionalities to prevent reentrancy attacks on both ERC20Staking and ERC721Staking contracts.

6.16 msg.value should be zero when staking ERC20 tokens

In the StakingERC20 contract, the staking token can either be an ERC20 token or the native gas token.

For gas tokens, funds are received through the payable operator of the stake(...) function. However, when the staking token is an ERC20 token, the function does not enforce msg.value to be zero. As a result, users may inadvertently send msg.value alongside the ERC20 token amount, potentially leading to the loss of funds.

function _stake(uint256 amount, uint256 lockDuration) internal {
    if (amount == 0) {
        revert ZeroValue();
    }

    // @audit msg.value must be 0 when stakingToken !=address(0)
    if (config.stakingToken == address(0)) {
        if (msg.value != amount) {
            revert InsufficientValue();
        }
    }

    Staker storage staker = stakers[msg.sender];

Recommendation

Add a check to ensure msg.value is 0 when config.stakingToken != address(0)

6.17 Errors declared in the interface but never used

The IStakingBase interface defines the following errors, but they are not used in any of the implementation contracts:

error InvalidStake()
error CannotClaim()

Recommendation

Remove the unused errors declaration.

6.18 Missing zero address check for contract admin

The ZeroVotingERC20 contract's constructor takes the admin address as an input. This address is granted the DEFAULT_ADMIN_ROLE, BURNER_ROLE, and MINTER_ROLE roles within the contract. However, the function does not check that this address is not set to address(0).

constructor(
        string memory name,
        string memory symbol,
        address admin
    ) ERC20(name, symbol)  ERC20Permit(name){
        //@audit Missing zero address check on admin
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
        _grantRole(BURNER_ROLE, admin);
        _grantRole(MINTER_ROLE, admin);
}

Recommendation

Implement a check in the constructor, ensuring admin cannot be set to address(0).

6.19 Redundant check for token ownership in _unstakeMany

The _unstakeMany(...) function processes an array of token IDs to be unstaked. For each token, the function checks whether the caller owns it and whether it was previously staked in the contract. This is done through the following checks:

if (
      nftStaker.staked[_tokenIds[i]] == false
      || IERC721(config.stakeRepToken).ownerOf(_tokenIds[i]) == address(0) //@audit Unnecessary check
      || IERC721(config.stakeRepToken).ownerOf(_tokenIds[i]) != msg.sender
  ) {
          // Either the list of tokenIds contains a non-existent token
          // or it contains a token the owner doesnt own
          unchecked {
              ++i;
          }
          continue;
}

However, the second check is redundant. Specifically, verifying that the token's owner is not address(0) is unnecessary, as the subsequent check already covers this condition, ensuring the token owner is msg.sender.

Recommendation

Remove the redundant check from the code above.

6.20 Redundant code when overriding the _getContractRewardsBalance function

The StakingERC20 contract inherits from StakingBase and overrides the _getContractRewardsBalance() function to account for staked tokens when calculating the total rewards in the contract. However, the first part of the function duplicates logic from the base contract’s overridden function.

To eliminate redundancy, this code can be replaced by calling super._getContractRewardsBalance() instead.

function _getContractRewardsBalance() internal view override returns (uint256) {
    uint256 balance;
    //@audit Following if-else block can be replaced by a call to `super._getContractRewardsBalance()`
    if (address(config.rewardsToken) == address(0)) {
        balance = address(this).balance;
    } else {
        balance = IERC20(config.rewardsToken).balanceOf(address(this));
    }

    if (config.rewardsToken == config.stakingToken) {
        return balance - totalStaked;
    }

    return balance;
}

Recommendation

Replace the redundant code in _getContractRewardsBalance() with a call to super._getContractRewardsBalance().

6.21 Unnecessary stakers mapping in StakingERC721 contract

The StakingBase contract implements common functionalities for staking both ERC20 and ERC721 tokens. It includes the stakers mapping, which is used to track each staker's data via the Staker struct.

contract StakingBase is Ownable, ReentrancyGuard, IStakingBase {
    ...
    /**
     * @notice Mapping of each staker to that staker's data in the `Staker` struct
     */
    mapping(address user => Staker staker) public stakers;
    ...
}

However, the stakers mapping is only used in the StakingERC20 contract. Since the mapping is declared in the base contract, it becomes part of the storage layout for the StakingERC721 contract as well, which results in redundancy and unnecessary storage usage.

Recommendation

Consider moving the stakers mapping to StakingERC20 interface and implementation contract.