OFT

In the SpellPowerStaking contract there is a lockup period where users are unable to exit their stakes if they have lastAdded to their stake within the lockupPeriod. However the lockup logic is only enforced for the stake function, and not for the stakeFor function as this is not overridden from the MultiRewards contract. Therefore users who stake through the cross-chain staking system which uses the BoundSpellActionReceiver as an operator to stake for a user address will not have their stakes locked for the lockupPeriod. Furthermore the whenNotPaused modifier is not applied to the stakeFor function as it is not overridden, so stakes through the cross-chain mechanism can still occur when the system is paused.

Override the stakeFor function in the SpellPowerStaking contract and apply the lastAdded storage write as well as the whenNotPaused modifier.

The BoundSpellActionReceiver contract interacts with the SpellPowerStaking contract via its stakeFor function. Specifically, when cross-chain actions are triggered through LayerZero, the _handleMintBoundSpellAndStake and _handleStakeBoundSpell functions call spellPowerStaking.stakeFor. However, the SpellPowerStaking contract enforces a lockup period for withdrawals via the lastAdded mapping, which tracks the last timestamp at which a user staked tokens. This lockup mechanism is implemented in the stake function of SpellPowerStaking, however, the stakeFor function in the MultiRewards contract (from which SpellPowerStaking inherits) is not overridden in SpellPowerStaking. As a result, when staking occurs through the BoundSpellActionReceiver using stakeFor, the lastAdded mapping is not updated. This allows users to bypass the lockup mechanism as they can stake without the lastAdded mapping being updated.

Override the stakeFor function in the SpellPowerStaking contract as shown below:

function stakeFor(address user, uint256 amount) public virtual override {
    super.stakeFor(user, amount);
    lastAdded[user] = block.timestamp;
}

There is a lockupPeriod that must pass after a stake before the user is able to withdraw in the SpellPowerStaking contract. The lastAdded mapping is updated on each stake, allowing the code to check whether the lockup period has passed on a withdrawal trial. However, stakeFor doesn't update that mapping. Any user can use the BoundSpellActionSender contract to send a stake message and make the BoundSpellActionReceiver call the stakeFor function. By doing so, they will successfully stake and avoid the lockup period.

Update the lockup period on stakeFor() as well.

The MultiRewards._getRewardsFor() function doesn't check if the current reward of the user for a given token is positive. This means a 0 transfer will be initiated for any rewardToken that hasn't accrued. If one of these tokens revert on 0 amount transfers, the given user will not be able to claim any rewards using that function until they accrue these rewards.

Consider executing a continue if the accrued rewards are 0 in both _getRewardsFor and MultiRewardsClaimingHandler.notifyRewards()

When a user calls _getRewardsFor() with RewardHandlerParams, their rewards will be sent to the MultiRewardsClaimingHandler contract which will send them to the desired destination chain. The receiver there will be the same address as the sender on the source chain. If the user doesn't have the same address on the other chain - because it's a smart contract or the other chain is not an EVM one, they will lose access to their rewards.

Consider letting the user pass a receiver address for the destination chain

The MultiRewards contract can be paused by its owner. When it's paused, no stakes are allowed. However, users may bypass that restriction by initiating a cross chain stake. The BoundSpellCrosschainAction will call the stakeFor function which doesn't check if the contract is paused or not. This will allow users to stake even in a paused state and potentially benefit from the current situation.

Consider disallowing stakeFor when the contract is paused.

The setLockupPeriod function can be used to extend the lockupPeriod without bound by the owner. This can be used to trap and hold hostage the funds of users who are deposited in the SpellPowerStaking contract. This may become a risk in the event that the owner address is compromised. Instead the configured lockup period could be limited to reduce this risk.

Consider adding an upper bound for the lockup period in the setLockupPeriod function.

The MultiRewards._getRewardsFor() function doesn't check if the current reward of the user for a given token is positive. This means a 0 transfer will be initiated for any rewardToken that hasn't accrued. If one of these tokens revert on 0 amount transfers, the given user will not be able to claim any rewards using that function until they accrue these rewards.

Consider executing a continue if the accrued rewards are 0 in both _getRewardsFor and MultiRewardsClaimingHandler.notifyRewards()

The contracts TokenLocker, MultiRewards, and SpellPowerStaking contain certain functions and permissions that could lead to centralization risks. These risks arise from the ability of privileged roles (such as the owner or operators) to perform actions that could potentially be detrimental to the interests of regular users. In the MultiRewards contract:

• Owner can never recover the stakingToken however he can recover any rewardToken.
• getRewardsFor function could be abused by an operator to send the accrued rewards to himself. In the SpellPowerStaking contract:
• Owner can extend the lockupPeriod indefinitely. In the TokenLocker contract:
• Operators have total control over the tokens within the contract. They are free to redeemFor, instantRedeemFor and claimFor on behalf of any user.

These centralization risks are noted to provide a clear understanding of the control dynamics within the contracts. No specific recommendations are provided as the intention is to document these aspects for awareness and consideration by stakeholders.

The MultiRewards contract assumes that all reward tokens have 18 decimals, as indicated by the use of 1e18 in calculations for rewards:

function rewardPerToken(address rewardToken) public view returns (uint256) {
    if (totalSupply == 0) {
        return _rewardData[rewardToken].rewardPerTokenStored;
    }

    uint256 timeElapsed = lastTimeRewardApplicable(rewardToken) - _rewardData[rewardToken].lastUpdateTime;
    uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply;

    return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
}

This assumption will lead to incorrect reward calculations if tokens with different decimal places are used.

Consider adding a check in the addReward function that enforces that the rewardToken added has 18 decimals.

Several functions in the contracts do not check for zero value transfers before attempting a token transfer. This can lead to failed transactions if a token reverts on zero value transfers which is a behavior exhibited by some custom ERC20 tokens. The functions affected are:

• MultiRewardsClaimingHandler.notifyRewards()
• MultiRewards._getRewardsFor(address user, RewardHandlerParams memory params)
• TokenLocker._instantRedeemFor() Moreover, in the MultiRewardsClaimingHandler.notifyRewards function, when there are no pending rewards for a specific rewardToken, the LayerZero's sendFrom method is still invoked to create a cross-chain message. This should not be done when the reward amount is zero.

Before any token transfer, add a check to ensure that the amount transferred is greater than zero. On the other hand, ensure that sendFrom is only called when the rewards amount is higher than zero.

The send function in the BoundSpellActionSender contract allows users to initiate cross-chain actions with a zero amount. This can lead to unnecessary transactions that consume gas without performing any meaningful operation.

Add a require check in the BoundSpellActionSender.send function that enforces that the amount sent is higher than 0.

In the TokenLocker contract the lockingDeadline parameter is passed to the redeem and _redeemFor functions but it does not seem to influence any logic within these functions. The parameter is not used to enforce any constraints, which makes it redundant in its current form. The only check performed is within the _createLock function which simply enforces that the provided lockingDeadline is higher or equal to the current block.timestamp.

Consider removing the lockingDeadline parameter from the TokenLocker contract.

The _updateInstantRedeemParams function in the TokenLocker contract updates the parameters for instant redemption, including the feeCollector address. However, there is no require check to ensure that the feeCollector address is not set to address(0). If the feeCollector is set to address(0), it can cause a denial of service in the insta-redemption process, as the instantRedeem function checks for a non-zero feeCollector address before proceeding.

Consider adding a require check in the _updateInstantRedeemParams function that enforces that the feeCollector address set is not the address(0).

In the TokenLocker contract, the _redeemFor function is responsible for redeeming a specified amount of tokens. However, before creating a new lock for the redeemed amount, the function calls _claim, which releases any accrued tokens to the to address. This behavior may not be expected by users who intend to redeem a specific amount to a particular address without affecting any accrued tokens.

Consider documenting the behavior of the _redeemFor function, informing users that it will also claim any accrued tokens to the to address. This will help users understand the full implications of using the function.

Multiple contracts within the system rely on Solady's SafeTransferLib library for token transfer operations. The version in use, as specified in the soldeer.lock file, is:

[[dependencies]]
name = "solady"
version = "0.0.231"
source = "https://github.com/Vectorized/solady.git"
checksum = "2907b5036b9b3489891e69b49adba24c793940d4"

This version of Solady is outdated. In a more recent update, introduced via PR#1128, an important security improvement was added to SafeTransferLib. The updated version includes a validation step to ensure that the token being transferred is a valid contract address with actual code. By using an outdated Solady’s version, the contracts miss out on this important safeguard. Upgrading to the latest version of Solady is recommended to enhance the security of any token transfer.

Consider using the latest Solady’s version.

Contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the protocol negatively. All the contracts in scope are using the following floating pragma: pragma solidity >=0.8.0;

Consider locking the pragma version in all the smart contracts. It is not recommended to use a floating pragma in production. For example: pragma solidity 0.8.20.

In the TokenLocker contract, the MathLib library is imported but not utilized anywhere within the contract.

Consider removing the import statement for the MathLib library in the TokenLocker contract.

The current ownership transfer process for all the contracts inheriting from the OwnableOperators contract involves the current owner calling the transferOwnership function:

function transferOwnership(address newOwner) external onlyOwner {
    owner = newOwner;
    emit OwnershipTransferred(msg.sender, newOwner);
}

If the nominated EOA account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyOwner modifier.

It is recommended to implement a two-step process transfer ownership process where the owner nominates an account and the nominated account needs to call an acceptOwnership function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. This can be easily achieved by using OpenZeppelin’s Ownable2Step contract.

All the contracts within scope, upon a token transfer, always assume that the full amount specified will be transferred to the recipient. However, fee-on-transfer tokens deduct a fee from the transfer amount, resulting in the recipient receiving less than expected. For example, tokens like USDT, while currently not implementing transfer fees, have the capability to do so. If a reward token with fees is ever added to the system, it could cause a total denial of service for the reward distribution process, as the expected and actual transferred amounts would not match.

Ensure that the contracts do not operate with fee-on-transfer tokens. In order to support this type of tokens, consider checking the balance before and after the token transfer to determine the total amount transferred after the fees were applied.

The MultiRewards contract may experience a minor accumulation of unclaimed reward tokens (dust) due to the inherent precision loss in Solidity's integer arithmetic. When calculating rewardPerToken and users' earned rewards, minimal precision loss occurs because of integer division truncating fractional values. This precision loss results in a small amount of rewards not being distributed, thus remaining in the contract. Over time, these minimal unclaimed amounts can accumulate within the contract, remaining inaccessible to users.

Consider utilizing the recover function to retrieve any accumulated dust if it becomes significant for any of the reward tokens.

ErrInvalidRewardHandler error in the MultiRewards contract is defined but never used.

Consider removing the error, or alternatively perform an input check during the setRewardHandler function and use the defined error.

As recommended by the LayerZero integration checklist point 5 & 6 (https://docs.layerzero.network/v1/developers/evm/evm-guides/advanced/relayer-adapter-parameters) the param for zroPaymentAddress and useZro should not be hardcoded. This prevents the ability of using the ZRO token as a fee payment option in the future.

Consider passing in an input parameter for the zroPaymentAddress and useZro field.

When users redeem, they burn their issued tokens and wait a certain amount of time to receive back the underlying token. They must wait for the current epoch to end plus the lockupPeriod. This means that the later they redeem their tokens in the given epoch, the less they will have to wait compared to users who redeemed earlier in the same epoch.

Consider documenting this behavior for users.

In the MINT_AND_STAKE_BOUNDSPELL action flow the MintBoundSpellAndStakeParams which is used includes a rewardHandlerParams entry where the data and value values are always hardcoded to "" and 0. These rewardHandlerParams values are never used during the execution of the mint and stake and therefore can be removed.

Consider removing the rewardHandlerParams from the MintBoundSpellAndStakeParams.

In the estimate function the packet type provided to the minDstGasLookup function is hardcoded as the literal 1.

Consider using the PT_SEND_AND_CALL constant value to follow best practices and avoid magic numbers.

MultiRewardsClaimingHandler.notifyRewards() populates the adapterParams field and then sends a message by calling sendFrom(). The sendFrom function will revert if the OFT's useCustomAdapterParams variable is set to false and users won't be able to claim their rewards to another chains.

Be sure to never toggle the useCustomAdapterParams, as it's currently set to true.

In the TokenLocker contract, the _redeemFor function is responsible for redeeming a specified amount of tokens. However, before creating a new lock for the redeemed amount, the function calls _claim, which releases any accrued tokens to the to address. This behavior may not be expected by users who intend to redeem a specific amount to a particular address without affecting any accrued tokens.

Consider documenting the behavior of the _redeemFor function, informing users that it will also claim any accrued tokens to the to address. This will help users understand the full implications of using the function.

In the TokenLocker contract, _releaseLocks function iterates through all the locks, releases the unlocked ones and updates the lastLockIndex. However, if all locks are released at the same time (e.g. a user waits long enough to claim all at once), this line will never hit, the lastLockIndex will not be updated and it will retain the previous lock count, even though the user no longer has any locks left.

Consider resetting lastLockIndex to 0 if the _userLocks[user].length becomes 0 after releasing locks.

When MultiRewardsClaimingHandler sends token to the destination chain, layerzero will not use all the tokens, but leave a dust amount of tokens in the contract, depending on the configured OFT decimals. Since there is no refund mechanism once the tokens enter the MultiRewardsClaimingHandler, users will always be losing these dust amounts. The same problem exists in BoundSpellCrosschainActions._sendMintAndStakeBoundSpell and BoundSpellCrosschainActions._sendStakeBoundSpell

Consider implementing a mapping with dustAmounts that the users can claim.

In_getRewardsFor, the rewardHandler contract is called as follows: rewardHandler.notifyRewards{value: params.value}(user, _rewards, params.data) However, params.value is user-defined and may differ from msg.value, leading to potential excess ETH left in the contract that cannot be retrieved. Furthermore, in notifyRewards if msg.value > param.fee + param.gas then excess ether is also stuck in contract.

Consider passing in msg.value instead of params.value:

rewardHandler.notifyRewards{value: msg.value}

The Create3Factory.deploy() function accepts a value parameter to use when deploying the new contract. However, there are no payable, receive or fallback functions in the Create3Factory contract which makes it impossible to send native token for deployment. Technically, it's still possible to send native token to the contract outside of the EVM, but anyone will be able to frontrun the deployment transaction and steal it by deploying their own contract and passing the balance of the Create3Deployer as value. In result, deployments which require sending funds will always fail.

Make the deploy function payable.

In the MultiRewardsClaimingHandler and BoundSpellCrosschainActions contracts, LayerZero/OFT calls are used to enable cross-chain operations. However, the procedure for managing scenarios where these messages fail to execute on the destination chain is not well-documented. This can result in some uncertainty regarding who is responsible for retrying or addressing failed transactions, whether it should be handled by a backend system or individually by the users.

Provide comprehensive documentation outlining the process for handling failed messages on the destination chain. Ensure that failed messages occur only under temporary circumstances, such as when the destination contract is paused. Permanent failed messages should never occur.

In notifyRewards, the user receiving rewards is also set as the refundAddress. However, if the user is a contract without a receive function then the refund mechanism will fail and prevent rewards from being claimed. Furthermore, if notifyRewards was called directly by an Operator (who pays the gas and fees), then the Operator should receive the refund instead of the user.

Consider allowing a function input for refundAddress instead of assuming it should be user.

The rewardTokens added to the MultiRewards contract can only be added and not removed. This is dangerous because:

• the array may become too large and DOS the system.
• one of the tokens may start reverting causing DOS of the system

Be aware of the risk.

When a user calls _getRewardsFor() with RewardHandlerParams, their rewards will be sent to the MultiRewardsClaimingHandler contract which will send them to the desired destination chain. The receiver there will be the same address as the sender on the source chain. If the user doesn't have the same address on the other chain - because it's a smart contract or the other chain is not an EVM one, they will lose access to their rewards.

Consider letting the user pass a receiver address for the destination chain

The MultiRewards contract assumes that all staking tokens have 18 decimals, as indicated by the use of 1e18 in calculations for rewards:

function rewardPerToken(address rewardToken) public view returns (uint256) {
    if (totalSupply == 0) {
        return _rewardData[rewardToken].rewardPerTokenStored;
    }

    uint256 timeElapsed = lastTimeRewardApplicable(rewardToken) - _rewardData[rewardToken].lastUpdateTime;
    uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply;

    return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
}

This assumption will lead to incorrect reward calculations if tokens with different decimal places are used.

Consider adding a check in the addReward function that enforces that the staking token used has 18 decimals.

In the notifyRewards function of MultiRewardsClaimingHandler contract, user provided params.gas amount is passed as gasLimit for the execution in the destination chain. This amount is then later checked in the LzApp._checkGasLimit, and it has to be greater than or equal to minDstGasLookup[_dstChainId][_type]. Even the non-blocking messaging channels can be blocked if the minDstGasLookup value is not sufficient enough. This can happen if the transaction reverts with Out Of Gas error before reaching the non-blocking app try/catch (e.g. somewhere here). As a result, failed transaction will be stored in Endpoint.storedPayload instead of LzNonblockingApp.failedMessages.

Ensure that the minDstGasLookup value is set sufficiently high to prevent failed messages from blocking the channel.

The LOCAL_CHAIN_ID for the current chain in MultiRewardsClaimingHandler is hardcoded to 0. If the user specifies 0 as dstChainId, the reward tokens will be simply transferred to them on the current chain. Typically, there is no reason for a user to go through that flow if they want to claim the reward tokens on the same chain, since they can do it by calling MultiRewards.getRewards() without passing any arguments. If for some reason an external integrator always call getRewards with parameters, it makes sense that when they want to claim on the same chain, they will pass its layer zero chain id as dstChainId and not 0. This will not be caught by the current if statement and the transaction will probably revert because minDstGasLookup for the current chain will not be set in the OFT. Because of this, the user will not be able to claim their tokens.

Consider having the LayerZero id on each chain as LOCAL_CHAIN_ID instead of hardcoding it to 0. You can additionally check for 0 as well if it's needed for some other integrations.

L-26 shows how in some cases a transfer of 0 amount can happen. It recommends to skip the current loop iteration if the current reward amount is 0 in both MultiRewards._getRewardsFor() and MultiRewardsClaimingHandler.notifyRewards(). The current implemented fix skips the iteration only in MultiRewards._getRewardsFor(). This fix introduces a more serious problem. In MultiRewards if the current reward is 0, the entry in the _rewards array won't be populated, i.e its token and amount will both be 0. The whole array will be passed to rewardHandler.notifyRewards(). Inside that function, there are two possible scenarios: A) transfer to the same chain B) transfer to a different chain Both of these cases will lead to revert if the current token is address(0). For A, safeTransfer() will be called on address(0). And for B, the oft will be retrieved as tokenOfts[address(0)] which will result in address(0) as well and will revert when sendFrom() is called. In result, core contract functionalities will be blocked for the given user until they accumulate rewards in each token listed.

Skip the iteration in MultiRewardsClaimingHandler.notifyRewards() if the current amount is 0.

In the _getRewardsFor function the _rewards array is now unpopulated when the reward amount is zero. As a result the rewardToken stored in the rewards entry will be address(0). This will lead to a DoS in the notifyRewards function in all cases. Firstly in the param.dstChainId == LOCAL_CHAIN_ID case will attempt to call the transfer function on the zero address which will revert. And in the param.dstChainId != LOCAL_CHAIN_ID case the transferFrom function is attempted to be called on the oft that corresponds to the token. The corresponding oft will also be the zero address and thus this call will subsequently revert.

Add early continue cases for both the token == address(0) and amount == 0 for robust compatibility with all operators and tokens which might revert on zero transfer.

In the _getRewardsFor function, if a particular reward amount is zero, the corresponding _rewards[i] entry remains unchanged and effectively defaults to address(0) and amount = 0. When the notifyRewards function subsequently processes the returned _rewards array, it attempts to perform a token transfer for that entry, causing the call to revert due to the invalid token address. This will disrupt normal reward distribution and prevent users from successfully receiving other valid rewards in the same batch.

Update the notifyRewards function to check for address(0) before attempting any token transfers. If the token address is address(0), simply continue to the next iteration.

In the stake and stakeFor functions the lastAdded mapping is updated after the super.stake and super.stakeFor calls. As a result, if the staking token allows any callback hooks upon transferFrom, then it may be possible for the user to re-enter into the SpellPowerStaking system and withdraw their stake immediately without waiting the lockup period. There is no identified incentive to do this, and the planned staking tokens do not have callbacks, however as a best practice the lastAdded should be updated before the call to super.stake and super.stakeFor.

Consider updating the lastAdded mapping before the calls to super.stake and super.stakeFor.

In the documentation for the BoundSpellActionReceiver onOFTReceived function the comment for the _payload parameter still references the rewardHandlerParams, however this value has been removed from the MintBoundSpellAndStakeParams struct.

Consider removing the comment that references the rewardHandlerParams.

The BoundSpellCrossChainActions file imports the RewardHandlerParams however these are not used in either contract in this file.

Remove the RewardHandlerParams import.

For the fix of L-25, the _getRewardsFor function now accepts a to address, allowing the reward recipient on the destination chain to be different from the user address on the source chain. The _exitFor and exit functions also accept a to address as a parameter to achieve this functionality.

    function _exitFor(address user, address to, RewardHandlerParams memory params) internal {
        _withdrawFor(user, balanceOf[user]);
        _getRewardsFor(user, to, params);
    }

During the call, the stakingToken is withdrawn to the user address, and rewards are transferred to to address. This behavior is intended as part of the fix for the previous L-25 issue. However, the same behavior is observed when the user calls the regular exit(address to, RewardHandlerParams memory params) function. This exit function gives the impression that all balances, including the stakingToken balance, will be transferred to the to address. However, only the rewards are transferred to the to address, while the stakingToken is still transferred to the user address.

If this behavior is intended, consider documenting it to avoid misunderstandings. Otherwise, transfer the stakingToken to the to address.

ErrZeroAddress error in MultiRewardsClaimingHandler contract is defined but never used.

Consider removing unused error.

rewardHandlerParams was removed from the MintBoundSpellAndStakeParams and is no longer used anywhere in the contract. However, the rewardHandlerParams import statement was not removed, and comments in line 237 still reference it.

Remove the rewardHandlerParams import statement and delete the comment that mentions rewardHandlerParams.