Push Chain Migration

The releaseVested function verifies if user already claimed by checking claimedvested[leaf] = true. As claimedvested is a boolean, there is no need to compare it to the true param.

Consider removing the comparison strict equality operator: if (claimedvested[leaf] )

The isClaimPaused state variable is defined in MigrationRelease.sol but it is never used. Additionally, the whenNotPaused modifier already handles this feature.

Remove isClaimPaused state variable.

bytes32 leaf = keccak256(abi.encodePacked(_recipient, _amount, _epoch));require(verifyAddress(_recipient, _amount, _epoch, _merkleProof) & instantClaimTime[leaf] = 0,"Not Whitelisted or already Claimed");

The leaf is locally constructed using _recipient, _amount, and _epoch, then passed to verifyAddress, which re-computes the same hash again. This is redundant and introduces unnecessary computation.

Consider computing the leaf once and passing it directly to the Merkle verification.

The explicit return statement at the end of this conditional block is unnecessary, as the function execution will naturally return at this point.

if (_token = address(0)) {transferFunds(_to, _amount);return;}

Consider removing the explicit return;

The Push team intends to prevent smart contracts from being recipient by checking the code size using the following logic:

uint256 codeLength;assembly {codeLength := extcodesize(_recipient) // @audit What is this codeLength check supposed to prevent?}

This check would block legitimate smart contract wallets (e.g., Gnosis Safe, ERC-4337 wallets) from interacting, which could limit usability.

Consider removing the extcodesize check.

The MigrationLocker is an upgradeable contract. Although some safety checks where enforced to avoid users initializing the implementation, nothing prevents a users from locking PUSH tokens in this contract by mistake. This is possible as the PUSH token is a constant and the contract is unpaused by default.

Consider calling _pause() in the constructor so future calls to the lock function in the implementation contract reverts.

The MigrationRelease uses SafeERC20 in the transfer function for recoverFunds. However, the MigrationLocker also contains a recoverFunds function but the SafeERC20 lib is not used.

Consider, using SafeERC20 library in the MigrationLocker contract.

Owner will call initiateNewEpoch to start a new epoch for locking PUSH tokens. This is an important state change in the contract that should emit an event to notify off chain services.

Consider emitting an epoch change event in the initiateNewEpoch function.

The fetchAndStoreEvents will fetch all Locked events between two blocks. This script is meant to be used once per epoch, at the end of each epoch.

If the locker contract is not paused before calling initiateNewEpoch, the following edge case might appear:

• epoch = 1, started at block 0
• Alice submits lock tx at block 100, Locked event uses epoch 1
• owner also submits initiateNewEpoch at block 100, epoch = 2
• team runs fetchAndStoreEvents filtering epoch 1 events.
• startBlock = epochStartBlock(1) = 0
• endBlock = epochStartBlock(1 + 1) - 1= 100 - 1 = 99
• locker.queryFilter will not include Alice's lock event.

Consider checking blocks up to and including the next block epoch's start block in the fetchAndStoreEvents script.

Currently, there is no validation in fetchAndStoreEvents to ensure that the sum of all leaves equals the actual amount locked on the contract for that epoch.

Theoretically, issues with the RPC (e.g., null values due to rate limits or internal errors) could result in the script omitting some lock events during Merkle root construction.

Since this is a one-way migration for the Push token itself, it is critical to include post-execution validations to ensure consistency with the on-chain state.

Consider tracking the amount locked for each epoch on-chain. After constructing the Merkle tree off-chain, sum the leaves and compare the result with the on-chain locked amount to verify that the off-chain state has been correctly captured.

The lock functions enforces users to first approve their PUSH tokens to be spent my the MigrationLocker contract. However, the PUSH token has a permit functionality, which can't be used in the locker contract.

Consider adding a function to lock tokens with permit signature.

relaese instead of release

Consider correcting it

The MigrationRelease contract emits the ReleasedInstant and ReleasedVested when users claim their PUSH tokens. These events emit the recipient's address, amount released, and epoch.

However, the natspec incorrectly states emits a ReleasedVested event with the recipient address, amount, and release time, but the epoch value is emitted instead.

Update the natspec to describe the correct parameters emitted in the release events, as the release time or block.timestamp will be included in the events anyways.

If a user locks a, claims a, and then locks b, the script currently tracks their total as a + b. This could allow them to claim twice in the same epoch.

Beware that there should be only one merkle root update per epoch.

The current natspec of the lock functions states: Emits a Locked event with the recipient address, amount, and a unique identifier

However, this was an old version of the contract, where an id was used in the Locked event, instead of the epoch value, which is not unique.

Update the natspec to correctly describe the parameters emitted in the Locked Event.

The README file key features states: Safety toggles to prevent/allow locking - Owner Controlled

However, the setToggleLock function was removed, and the lock is now based on the pause/unpause functionality

Make sure README clearly addresses the features contained in the smart contracts.

A tree is constructed for each user, and then again twice for both getProof and verify in proofArray.js.

This is incredibly inefficient, as the tree remains the same for each user.

Consider constructing the tree once and passing it as an object instead.

event NewEpoch(uint256 indexed epoch, uint256 indexed startBlock);

This event will only be emitted once per epoch, which will likely be a different block number.

Remove the indexed event params.

Push has added strict balance validations in the fetch script.

However, if even 1 wei is accidentally or deliberately sent (donated) to the contract, these validations may cause the script to revert or throw an error.

Be aware of this edge case during debugging or triage.

If a throw is observed, consider the possibility of donations as a potential cause.