Kwenta A-7

Reference: RewardEscrowV2.sol#L405, RewardEscrowV2.sol#L410

Once migration starts, treasuryDAO address in RewardEscrow.sol is set to the address of the EscrowMigrator contract, thus transferring all early vesting fees to the migrator. This is required for users who want to migrate their escrowed KWENTAs to the V2 contracts.

However, if a user has no intention to migrate, but only wants to vest the escrowed KWENTAs (without registering any entries on the migrator), any vesting fees will be transferred to the migrator contract where there is no possibilities to withdraw those KWENTAs and give it back to the treasuryDAO.

Remediations to Consider

Consider adding a function to EscrowMigrator.sol that allows to withdraw leftover KWENTAs that haven't been registered.

Note: The Kwenta team was already aware of this issue and had considered the plan of upgrading and sweeping the contract at a later date. After the recommendation from the Macro team, Kwenta decided to fix the issue preemptively.

Reference: EscrowMigrator.sol#L92-L93

EscrowMigrator.sol checks for zero address in the constructor for the parameters _rewardEscrowV1 and _rewardEscrowV2, but is missing the zero address check for the staking parameters:

• _stakingRewardsV1
• _stakingRewardsV2

Remediation to Consider

Consider adding the missing zero address checks for above parameters.

Reference: RewardEscrowV2.sol#L296

In RewardEscrowV2, getVestingQuantity doesn’t work with duplicate entryIDs. Off-chain components and other contracts can call this function and might unintentionally pass duplicate entryIDs into the _entryIDs array. As getVestingQuantity double counts duplicate entries, a caller can make false assumption based on the wrong total and totalFee values returned.

Remediation to Consider

Consider handling duplicate entryIDs appropriately.

Reference: StakingRewardsV2.sol#L509, StakingRewardsV2.sol#L519, StakingRewardsV2.sol#L529

In StakingRewards.sol, the function balanceAtTime, escrowedbalanceAtTime, and totalSupplyAtTime can be used to retrieve the balance at a certain time in the past.

However, when those functions are called with a timestamp that equals the current block timestamp, then the functions might return inaccurate values as further transactions changing the balances can still occur within the same block.

Remediation to Consider

Consider reverting with an appropriate error if a timestamp is passed that equals block.timestamp. If this is a scenario that should be supported, consider documenting it appropriately to prevent misinformation.

Since Solidity 0.8.8, when implementing an interface, those functions don't require the override keyword anymore. In fact, this is misleading, as the function don't actual override any parent functions.

Consider removing the override keyword from functions that only implement the function definition from the derived interface.

• In EscrowMigrator.sol#L51, the comment says:

  /// @notice Contract for RewardEscrowV1
  

  But it should be RewardEscrowV2 instead.

• In RewardEscrowV2.sol#L64, the comment says:

  /// @notice Contract for StakingRewardsV2
  IEscrowMigrator public escrowMigrator;
  

  But it should be EscrowMigrator instead of StakingRewardsV2.

In RewardEscrowV2.sol, getVestingSchedules reverts with InvalidIndex when endIndex < index, while getAccountVestingEntryIDs returns an empty array in this case.

Consider implementing consistent behavior across those two functions.

In StakingRewardsV2.sol, IERC20 is imported from SafeERC20.sol:

import {IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

However, SafeERC20 is not used anymore in V2 and thus it is preferred to directly import from IERC20.sol interface:

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

In StakingRewardsV2.sol, RewardEscrowV2.sol, and EscrowMigrator.sol, the constructor arguments are missing natspec param descriptions. Consider adding proper natspec param descriptions for the constructor arguments.

In StakingRewardsV2.sol, functions and state variables use the lowerCamelCase naming convention. However, this is not the case for the newly introduced function escrowedbalanceAtTime, which uses a lower b for balance (as opposed to e.g. escrowedBalanceOf).

Consider renaming the function to escrowed**B**alanceAtTime to follow the naming convention.

In StakingRewardsV2.sol, the following Checkpoint struct is used:

struct Checkpoint {
        // The timestamp when the value was generated
        uint256 ts;
        // The block number when the value was generated
        uint256 blk;
        // The value of the checkpoint
        uint256 value;
    }

In its current state, Checkpoint.blk is not required anywhere in the code and checkpointing entirely relies on the timestamp Checkpoint.ts.

Consider removing blk from Checkpoint struct to save gas.