EscrowMigrator Code Style Findings

EMR-01C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		EscrowMigrator.sol:L143, L160, L172, L191-L192, L223, L225, L262, L305, L313, L317, L357

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

Copy
141for (uint256 i = 0; i < length; i++) {
142    uint256 entryID = entries[i];
143    VestingEntry storage entry = registeredVestingSchedules[_account][entryID];
144    if (entry.migrated) total++;
145}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation:

All relevant mapping lookups have been optimized to the greatest extent possible, optimizing the gas costs of the contract across the board significantly.

EMR-02C: Inexplicable Value Literal

Type	Severity	Location
Code Style		EscrowMigrator.sol:L380

Description:

The referenced value literal is inexplicable as the RewardEscrowV2 contract exposes its DEFAULT_EARLY_VESTING_FEE member publicly.

Example:

Copy
376IRewardEscrowV2.VestingEntry memory entry = IRewardEscrowV2.VestingEntry({
377    escrowAmount: originalEscrowAmount,
378    duration: duration,
379    endTime: endTime,
380    earlyVestingFee: 90
381});

Recommendation:

We advise the said value to be utilized, ensuring that the EscrowMigrator remains compatible with RewardEscrowV2 upgrades.

Alleviation:

The Kwenta team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase.

EMR-03C: Loop Iterator Optimizations

Type	Severity	Location
Gas Optimization		EscrowMigrator.sol:L141, L158, L170, L301, L352

Description:

The linked for loops increment / decrement their iterator "safely" due to Solidity's built - in safe arithmetics (post-0.8.X).

Example:

Copy
141for (uint256 i = 0; i < length; i++) {

Recommendation:

We advise the increment / decrement operations to be performed in an unchecked code block as the last statement within each for loop to optimize their execution cost.

Alleviation:

The optimization has been applied on all referenced for loops that do not contain continue statements as such statements would render the codebase convoluted due to the need to introduce unchecked increments of the iterator in multiple points of each loop.

As such, we consider this exhibit fully alleviated given that we agree with Kwenta's assessment.

EMR-04C: Potential Enhancements of Contract's Functionality

Type	Severity	Location
Code Style		EscrowMigrator.sol:L33

Description:

The EscrowMigrator appears to not account for any early vesting fees that are sent to it and not necessarily migrated. Additionally, users may overpay when migrating their entries due to mis-use of the contract as explained in the project's documentation.

Example:

Copy
24/// @dev WARNING: There is a footgun when using this contract
25/// Once a user is initiated, any entries they vest BEFORE registering, they will have to pay extra for
26/// Once again:
27/// If a user vests an entry after initiating without registering it first, they will have to pay extra for it

Recommendation:

While the latter trait may be undesirable, we propose an expiry system to be put in place enforcing a deadline up to which migrations are feasible. This deadline may coincide with the last rewards of the StakingRewards contract and would permit the Kwenta team to extract any early vest fees that were redirected but not claimed as part of the migration process (i.e. due to users never registering their entries in the first place).

Additionally, to aid in the recovery of mis-sent funds we advise a Merkle-Proof system to be put in place after the migration process deadline that will allow users to claim any funds they had to over-pay to migrate their entries.

Alleviation:

A 2 week migration deadline per account has been enforced in the codebase and an excess fund recovery function has been introduced, permitting funds that improperly end up in the contract to be "rescued" to the treasuryDAO, alleviating this exhibit in full.

EMR-05C: Significant Optimization of Contract's Storage

Type	Severity	Location
Gas Optimization		EscrowMigrator.sol:L314

Description:

The VestingEntry structures are meant to retain the escrowed amount to be migrated as well as whether the particular entry has been migrated. As a bool variable only requires 8 bits of space in the EVM, it is possible to downgrade the escrowAmount variable to an uint248 variable, tightly packing it with the bool variable migrated and thus reducing the storage footprint of each VestingEntry from two 32-byte slots to one 32-byte slot.

Example:

Copy
313registeredVestingSchedules[_account][entryID] =
314    VestingEntry({escrowAmount: escrowAmount, migrated: false});

Recommendation:

We advise this optimization to be applied as realistically no escrowAmount value will exceed the maximum of an uint248 variable.

Alleviation:

The data type of the escrowAmount member of a VestingEntry has been reduced to the uint248 data type, optimizing the code's storage needs significantly.

EMR-06C: Worst Case Optimization of Getter Function

Type	Severity	Location
Gas Optimization		EscrowMigrator.sol:L214, L254

Description:

The EscrowMigrator::getRegisteredVestingSchedules function will yield an empty array if endIndex is equal to _index, however, it performs multiple extra steps to do so unnecessarily.

Example:

Copy
214if (endIndex < _index) return new VestingEntryWithID[](0);
215
216uint256 n;
217unchecked {
218    n = endIndex - _index;
219}
220
221VestingEntryWithID[] memory vestingEntries = new VestingEntryWithID[](n);
222for (uint256 i; i < n;) {
223    uint256 entryID = registeredEntryIDs[_account][i + _index];
224
225    VestingEntry storage entry = registeredVestingSchedules[_account][entryID];
226
227    vestingEntries[i] = VestingEntryWithID({
228        entryID: entryID,
229        escrowAmount: entry.escrowAmount,
230        migrated: entry.migrated
231    });
232
233    unchecked {
234        ++i;
235    }
236}
237return vestingEntries;

Recommendation:

We advise the same approach as EscrowMigrator::getRegisteredVestingEntryIDs to be applied, converting the referenced comparator to be inclusive (i.e. <=) and thus optimizing the code's worst-case gas cost.

Alleviation:

The referenced if statement has been made inclusive, optimizing the code's worst-case gas cost and thus addressing this exhibit.