thirdweb A-7

In all three staking contracts, administrator role members (DEFAULT_ADMIN_ROLE) have permission to set and change the staking parameters rewardsPerUnitTime and timeUnit.

By setting timeUnit = 0, the _calculateRewards() logical math would try to execute a division by zero and, by setting rewardsPerUnitTime to a high value such as type(uint256).max, the operation will result in an overflow. Both of these parameter configurations will make every attempt of the user to withdraw, stake new tokens, or claim rewards revert, essentially breaking the entire contract and locking the existent tokens in the staking contract.

In the latest case, the value of rewardsPerUnitTime could be less than the max value, depending on the time lapsed and the amount staked.

function _calculateRewards(address _staker) internal view virtual returns (uint256 _rewards) {
  Staker memory staker = stakers[_staker];
  _rewards = ((((block.timestamp - staker.timeOfLastUpdate) * staker.amountStaked) * rewardsPerUnitTime) / timeUnit);
}

These conditions are irreversible: all subsequent parameter changes will revert due to the reward update logic.

Remediations to Consider

For timeUnit= 0:

• Requiring the timeUnit value to be always higher than zero.

For rewardsPerUnitTime:

• Executing an overflow check and ignoring the rewards result if overflow.
• Allowing users to withdraw no matter the staking conditions.

The documentation for setRewardRatio() explains:

Set rewards per unit of time. Interpreted as (numerator/denominator) rewards per second/per day/etc based on time-unit. For e.g., ratio of 1/20 would mean 1 reward token for every 20 tokens staked.

This is correct in cases where the staking and reward tokens have identical decimal values. Otherwise, rewards will be over- or under-awarded by orders of magnitude equivalent to the decimal difference.

Remediations to Consider

Consider accounting for both token decimals in reward calculations.

If a token with on-transfer fees is set up as the staked token, the stake() function will store an inaccurate amount. As a result:

• Rewards will be calculated from higher staked amounts than actually present in the contract.
• Returned amount from getStakeInfo() will not reflect the actual balances of the contract and staked amount.
• Withdrawals of the full staked amount will transfer non-corresponded tokens from other stakers.
• For a TokenStake with a unique stake, calling withdraw with the amount returned from getStakeInfo() will revert. To claim rewards the user would have to manually check the balances in the contract and use the actually transferred tokens.

Remediations to Consider

Consider, in the _stake() logic, using the balance of the contract before and after transferring the tokens to account for the corresponding amounts per staker.

This applies to all three staking implementations. The admin will no longer be able to call these functions, preventing the admin from increasing / decreasing / halting staking reward configuration. The admin may be forced to continue to support the present configurations, or halt funding entirely: in this case users will have no on-chain signal that rewards are no longer supported. This can increase the likelihood of cases where user unclaimedRewards exceeds budgeted supply, resulting in reward loss.

This occurs because these methods calculate and update rewards for every staker in stakerArray. The gas cost will exceed the current block gas limit of 30M when staker counts climb past:

• ~1,000 for TokenStake.sol
• ~1,000 for NFTStake.sol
• ~725 for EditionStake.sol

Note that the transaction gas cost may become prohibitively high for admin users well before the block gas limit is reached, with fewer stakers.

Remediations to Consider

Consider altering the staking and rewards logic to not require state modification for every staker. One such reference implementation is the Synthetix StakingRewards.sol contract.

Alternatively, consider including these limitations in developer documentation, potentially also noting increasing gas costs as these limits are approached.

Once transferred, the token cannot be recovered via withdraw(). This occurs because NFTStake.sol supports onERC721Received() which allows any safe transfer to succeed, but only correctly accounts for the staking if stake() is used. Similar conditions exist for EditionStake.sol.

This finding is not concerned with non-safe transfers of ERC721 tokens: these cannot be prevented, and are generally discouraged when sending tokens to a new address.

Remediations to Consider

Consider implementing a state variable toggle to allow safe transfers only from within stake(). A generalized example:

contract ... {
    uint8 private isStaking = 1;

    ...

    function _stake(...) ... {
    ...
        isStaking = 2;
        IERCx(...).safeTransferFrom(...);
        isStaking = 1;
    ...
    }

    function onERCxReceived(...) {
        require(isStaking == 2);
        ...
    }
}

Note that some method locations may need to be refactored to pursue this approach.

A double entry-point ERC20 Token is an ERC20 with a proxy pattern that allows users (and contracts) to interact directly with the target contract, skipping the proxy. Since it is possible to interact with both the proxy and the target directly, the token has two entry points.

By setting a TokenStake contract with both these addresses as reward and token, admins could drain rewards and staked tokens equally.

This issue was presented a few months ago with every Synthetix asset and the TUSD contract and was later patched after realizing several interactions with DeFi contracts caused vulnerabilities. Naturally, this does not mean that this vulnerability can never arise again, as there might be more tokens out there with multiple entry points.

Remediations to Consider

Consider adding a state to track the deposited rewards with a pull mechanism and add or subtract when adding/claiming rewards to keep rewards and staked amounts independent.

ERC165 is not correctly implemented, which will yield false when these contracts are queried for the interfaces they are intended to support. This may block some token transfers.

Remediations to Consider

Per the ERC165 spec, consider updating supportsInterface() to also return true when interfaceId is 0x01ffc9a7.

Staking721Upgradeable.sol does not use safeTransferFrom() within stake(), rendering NFTStake.sol’s onERC721Received() unused.

Remediations to Consider

Consider updating stake() logic to call safeTransferFrom().

The prebuilt contracts NFTStake.sol, EditionStake.sol, TokenStake.sol partially support trusted forwarders via _msgSender(), but their dependencies do not (e.g. Permissions.sol, Staking721Upgradeable.sol, etc). Notably, attempts to call Permissions.sol methods: grantRole(), revokeRole(), renounceRole(), or the onlyRole() modifier on NFTStake, EditionStake, TokenStake (or contracts derived from them) via trusted forwarder will apply the changes to the trusted forwarder address, not the intended transaction originator. This could have unintended consequences, such as inadvertently granting admin privileges to all transactions sent through a forwarder.

Consider implementing support for _msgSender() in all dependency contracts. OpenZeppelin does this by adding Context as a common base class. We recommend something similar. If this is not possible in the short term, we recommend pulling ERC2771 support out from these implementations of TokenStake, NFTStake, EditionStake until a more wholistic solution is adopted.

__ReentrancyGuard_init() is being called both in the parent (Staking721Upgradeable.sol, Staking1155Upgradeable.sol, and Staking20Upgradeable.sol) and child (NFTStake.sol, EditionStake.sol, and TokenStake.sol) contract initialize() functions. Consider removing this initialization call from the NFTStake.sol, EditionStake.sol, and TokenStake.sol contracts.

Staking721Upgradeable.sol — and the ERC20 and ERC1155 counterparts — implement the following identical logic for updating a single user’s unclaimed rewards within both _updateUnclaimedRewardsForStake() and _updateUnclaimedRewardsForAll() :

uint256 rewards = _calculateRewards(_staker);
stakers[_staker].unclaimedRewards += rewards;
stakers[_staker].timeOfLastUpdate = block.timestamp;

A user who customizes _updateUnclaimedRewardsForStake() must also remember to customize _updateUnclaimedRewardsForAll(), otherwise unexpected defects may be introduced.

Rather than duplicating logic, consider calling _updateUnclaimedRewardsForStake() from _updateUnclaimedRewardsForAll().

Admins can mistakenly call these functions with the same value already set, unnecessarily incurring considerably high gas costs, depending on the number of stakers. To avoid this, consider requiring different input data from the already stored values.

This view function performs multiple loops on unbounded arrays. For an external view function it won’t be an issue, but since the function visibility is public it could suggest the use of it from inside the contract. Consider updating this method to be external.

Staking contracts provide no ready insight into reward balances or withdrawals. Users will have to locate and inspect transaction activity of the reward token contract to confirm availability of rewards.

Consider increasing the availability of reward metadata on-contract. For example:

• Adding a view method to return present reward balance
• Emitting an event when withdrawRewardTokens() is called.

For ERC1155 staking contracts (EditionStake.sol and Staking1155Upgradeable.sol), the claimRewards() function differs from the other two staking contracts; this one is done for each tokenId individually, different from claiming all rewards with one call the other contracts have. This implies every user would need to execute one transaction for each tokenId staked, causing high amounts of gas and actions for a non-fungible ERC1155 design.

EditionStake.sol supports multi-call, which might be a solution when using that contract specifically, but not for contracts inheriting from Staking1155Upgradeable.

Consider having a function for claiming all rewards for the total amount of tokens staked and one for individual claims per token ID if desired.

See Staking721Upgradeable.sol:

for (uint256 i = 0; i < stakersArray.length; ++i) {
    if (stakersArray[i] == msg.sender) {
        stakersArray[i] = stakersArray[stakersArray.length - 1];
        stakersArray.pop();
    }
}

This will cycle over the entire stakerArray, even after the intended operation is completed. Consider calling break at the end of the if block, as is implemented in the other two staking contracts (Staking1155Upgreadable.sol and Staking20Upgreadable.sol).

In Staking721Upgreadable.sol, line 213, we have the following logic inside the _withdraw() function:

for (uint256 i = 0; i < stakersArray.length; ++i) {
    if (stakersArray[i] == msg.sender) {
        stakersArray[i] = stakersArray[stakersArray.length - 1];
        stakersArray.pop();
    }
}

Both stakersArray.length and starkersArray[i] inside the if statement can be optimized by storing them in memory, as is currently implemented in the other two staking contracts (Staking1155Upgreadable.sol and Staking20Upgreadable.sol).

Another optimization is to use the stakersArray[i] = stakersArray[stakersArray.length - 1] from memory too, this can be added to all three contracts.

Admins that want to change both values will need to call each method separately, which will both invoke _updateUnclaimedRewards(), which can become expensive as the number of stakers increase.

Consider implementing an additional method to update both settings at once, requiring only a single call to _updateUnclaimedRewards().