EscrowMigrator Manual Review Findings

EMR-01M: Inexistent Sanitization of Array Length

Type	Severity	Location
Input Sanitization		EscrowMigrator.sol:L288

Description:

The EscrowMigrator::_registerEntries function does not sanitize its _entryIDs array, permitting an empty array to be specified and thus initiate the user without actually registering any entries.

Example:

Copy
288function _registerEntries(address _account, uint256[] calldata _entryIDs)
289    internal
290    whenNotPaused
291{
292    if (!initiated[_account]) {
293        if (stakingRewardsV1.earned(_account) != 0) revert MustClaimStakingRewards();
294        if (rewardEscrowV1.balanceOf(_account) == 0) revert NoEscrowBalanceToMigrate();
295
296        initiated[_account] = true;
297        escrowVestedAtStart[_account] = rewardEscrowV1.totalVestedAccountBalance(_account);
298    }
299
300    uint256 registeredEscrow;
301    for (uint256 i = 0; i < _entryIDs.length; i++) {

Recommendation:

We advise a length requirement to be specified ensuring that the array is not empty.

Alleviation:

The Kwenta team evaluated this exhibit and opted to retain the current behaviour in place as they deem it desirable behaviour for a user to be able to "register" with zero entries. As such, we consider this exhibit nullified.

EMR-02M: Improper Deployment Methodology

Type	Severity	Location
Language Specific		EscrowMigrator.sol:L109-L119

Description:

The EscrowMigrator contract is meant to be deployed and be set as the treasuryDAO of the RewardEscrow contract in order to receive early vest fees. As such, its default state should be "paused".

Impact:

Although highly unlikely due to the usage of modern deployment practices, should the EscrowMigrator be utilized by even a single user before V1 is properly set up to redirect early vest fees to it, the accounting system of the contract will become corrupted as it will be at a loss of the early vest fees of the first user's registered entries.

Example:

Copy
108/// @inheritdoc IEscrowMigrator
109function initialize(address _contractOwner) external override initializer {
110    if (_contractOwner == address(0)) revert ZeroAddress();
111
112    // Initialize inherited contracts
113    __Ownable2Step_init();
114    __Pausable_init();
115    __UUPSUpgradeable_init();
116
117    // transfer ownership
118    _transferOwnership(_contractOwner);
119}

Recommendation:

We advise the contract to pause itself in the EscrowMigrator::initialize function, ensuring that it is not interacted with until all preparations on V1 have been completed.

Alleviation:

The EscrowMigrator::initialize function was updated to properly initialize the contract in a paused state via the PausableUpgradeable::_pause function, alleviating this exhibit.

EMR-03M: Inexistent Application of Checks-Effects-Interactions

Type	Severity	Location
Language Specific		EscrowMigrator.sol:L365, L386

Description:

The EscrowMigrator::_migrateEntries function will validate that an entry has not been migrated yet, will migrate it, and then set it as "migrated". This pattern is ill-advised as any re-entrancy during the external interactions in between the validation and the adjustment of the migrated flag could exploit the system.

Impact:

While not an active security threat, it may latently arise in a future upgrade of the RewardEscrowV2 which could significantly affect funds held by the EscrowMigrator contract.

Example:

Copy
364// skip if already migrated
365if (registeredEntry.migrated) continue;
366
367/// @dev it essential for security that the duration is not less than the cooldown period,
368/// otherwise the user could do a governance attack by bypassing the unstaking cooldown lock
369/// by migrating their escrow then staking, voting, and vesting immediately
370if (duration < cooldown) {
371    uint256 timeCreated = endTime - duration;
372    duration = cooldown;
373    endTime = uint64(timeCreated + cooldown);
374}
375
376IRewardEscrowV2.VestingEntry memory entry = IRewardEscrowV2.VestingEntry({
377    escrowAmount: originalEscrowAmount,
378    duration: duration,
379    endTime: endTime,
380    earlyVestingFee: 90
381});
382
383kwenta.transfer(address(rewardEscrowV2), originalEscrowAmount);
384rewardEscrowV2.importEscrowEntry(_to, entry);
385
386registeredEntry.migrated = true;

Recommendation:

While no re-entrancy can presently occur, the RewardEscrowV2 contract may utilize a "safe" minting procedure as defined by the EIP-721 standard which would be re-entrant. To this end, we advise the migrated flag to be set to true right after its validation as the code can only fatally fail in the ensuing statements which would revert the state change properly.

Alleviation:

The code's statements were re-ordered per our recommendation, ensuring that a registered entry is marked as migrated immediately after being evaluated as not being so, conforming to the CEI pattern and alleviating this exhibit.

EMR-04M: Overly Restrictive Pre-Condition

Type	Severity	Location
Language Specific		EscrowMigrator.sol:L293

Description:

The EscrowMigrator::_registerEntries function will mandate that the _account registering the entries does not have any earned rewards in the StakingRewards contract.

While this restriction would function fine in a test environment, in a production environment non-negligible time may elapse between a call to StakingRewards::getReward and EscrowMigrator::registerEntries.

The time elapsed between those two calls would be sufficient for the earned reward of the user to go to a non-zero value (depending on how much liquid balance they have staked) and as such, it may prove impossible to properly register their entries in time.

Impact:

The EscrowMigrator may prove unusable in a production environment where multiple minutes may elapse between transactions due to block confirmation times, delays in user actions, multi-signature wallet interactions, and other such real-life conditions.

Example:

Copy
288function _registerEntries(address _account, uint256[] calldata _entryIDs)
289    internal
290    whenNotPaused
291{
292    if (!initiated[_account]) {
293        if (stakingRewardsV1.earned(_account) != 0) revert MustClaimStakingRewards();
294        if (rewardEscrowV1.balanceOf(_account) == 0) revert NoEscrowBalanceToMigrate();
295
296        initiated[_account] = true;
297        escrowVestedAtStart[_account] = rewardEscrowV1.totalVestedAccountBalance(_account);
298    }

Recommendation:

We advise the code to either instruct users to fully exit the StakingRewards contract or to apply a "dust" threshold in the earned rewards of the account rather than ensuring the earned rewards are exactly 0.

We consider either of the two approaches as appropriate remediations to this exhibit.

Alleviation:

After discussions with the Kwenta team, we assessed that the contract is intended to be utilized for migrations once the reward distributor no longer disburses funds to the stakingRewardsV1 member.

Based on this deployment assumption, we consider this exhibit to be nullified as it would only manifest if the stakingRewardsV1 continued distributing rewards to its stakers as the migration process was taking place.