Voting Escrow Updates

The schedules mapping inside MerkleVester is lazily stored as acknowledged by code comments. Therefore the withdrawal address within schedules is initially address(0) and is updated via the _checkOrSetOriginalBeneficiary() function. This function is used in three different places mainly:

1. During delegation related calls:
   1. Delegation related calls (delegateFunds(), reclaimDelegatedFunds(), withdrawEarned()) calls _validateEscrowRequest() which in turn this function sets the withdrawal address in schedule before any operation correctly.
2. Changing beneficiary call:
   1. transferBeneficiaryAddress() function also correctly sets the withdrawal address in schedule before changing it.
3. Withdrawal request:
   1. withdraw() call continue with internal withdrawal calls according to the type of the vest. In both internal functions however (_withdrawCalendar(), and _withdrawInterval()) setting withdraw address comes after withdraw address check: if ((schedules[calendar.allocation.id].withdrawalAddress != msg.sender) && !adminWithdrawFeature) revert UnauthorizedWithdrawal();

Hence when adminWithdrawFeature is false, users cannot directly do withdrawals. In order to withdraw their funds in this scenario, the user has to either first delegate to escrow and then reclaim it so that their withdraw address will be recorded. However, if the escrow feature is not enabled, then the user cannot even delegate funds.

An admin can potentially transfer the beneficiary, but this is only possible if transferableByAdmin is enabled and unfavorable when there are numerous allocations. Ultimately, user withdrawals from the vester are prevented, a core functionality of the protocol.

Inside _withdrawCalendar and _withdrawInterval, call function _checkOrSetOriginalBeneficiary prior to if ((withdrawalAddress != msg.sender) && !adminWithdrawFeature) revert UnauthorizedWithdrawal();

The benefactor can revoke an allocation, such that funds are reclaimed and send to the vester contract, and withdrawals for the allocation by the beneficiary are prevented. In the case that the revokeAllFeature is disabled, the benefactor will be unable to retrieve these funds, since both functions defund and revokeAll rely on the feature being enabled.

Function rescueTokens will revert since the token being vested cannot be rescued: if (_errantTokenAddress == token) revert InvalidToken(); Ultimately, revoked funds are stuck in the MerkleVester contract.

Consider removing the revokeAllFeature from the defund function entirely, or at minimum making a separate feature type for defund and documenting this behavior.

When benefactor revoke's someone vesting, their terminatedTimestamp is changed so that they can not withdraw any funds (their withdrawable will be 0) as can be seen: withdrawableschedules[allocationId].terminatedTimestamp = uint32(1);

The problem is that revoked users can still delegate the revoked funds. When the revokeAllFeature is enabled, the benefactor has the capability to remove specified amount of funds from vesting. The normal workflow for revocations can be assumed as: first revoke and then defund revoked amount.

When this happens, revoked users will have nothing as withdrawable and then the funds allocated to them initially will be removed from contract. However, since these users can still delegate funds that is recorded inside Allocation struct, they essentially will be able to delegate funds that do not belong to them which results in blocking other user's ability to delegate and withdraw because of insufficient funds in the contract.

Furthermore, allowing delegation after revoke or cancel can lead to funds stuck in the escrow contract forever. The allocation cannot be revoked or cancelled again to reclaim those funds as the terminatedTimestamp is non-zero. If revokeAllFeature is false (not ideal if true as all allocations will be revoked) and transferableByAdmin is false then benefactor can't reclaim those funds and they are stuck in the escrow contract.

Validate that the terminatedTimestamp for the allocation is zero inside function delegateFunds.

With the rescueTokenFromEscrow function the benefactor may withdraw delegation rewards from any delegator contract where tokens other than the delegation token have been rewarded.

However these rewards ought to belong to the beneficiary who delegated to receive those rewards, the benefactor should not be able to take them.

Consider allowing potential reward tokens to be blacklisted from the rescueTokenFromEscrow function. Otherwise modify the rescueTokenFromEscrow function such that only beneficiaries are able to rescue funds from their delegation contract.

Certain rebasing tokens can not only increase in balance, but also decrease. For such tokens such as Ampleforth, the following validation in the EscrowVester could revert after rebases: if (_escrowedFunds > IERC20(escrowToken).balanceOf(address(delegateAddress))) revert UnauthorizedDelegation();

This can lead to a situation where a user delegates but after a rebase is unable to _reclaimFunds.

Clearly document to users that rebasing tokens are not fully supported.

Inside the internal withdraw functions, the following validation is performed:

if ((schedules[calendar.allocation.id].withdrawalAddress != msg.sender) && !adminWithdrawFeature) revert UnauthorizedWithdrawal();

This check does the following:

1. If adminWithdrawFeature is false, only let msg.sender call withdraw()
2. If adminWithdrawFeature is true, don't revert.

While the purpose of the validation was to give permission only to the benefactor (admin) in the case that adminWithdrawFeature is enabled, it actually gives permission to everyone.

Since withdraw calls reclaims all funds from delegation escrow, the delegatee will lose all their voting power that comes from escrow. This prevents voting power from being delegated and maintained, which is a core functionality of the escrow updates. The withdrawals can be 1 wei and done continuously, as all funds are reclaimed from the escrow contract regardless of the requested withdrawal amount.

If msg.sender is not beneficiary but admin feature is enabled, perform a benefactor check.

In the cancel function the getLeafJustAllocationDataWithFullyUnlockedCheck function is used to perform validation on the fully unlocked timestamp.

However this function is invoked twice, when the validation from the first invocation is sufficient.

Remove the second invocation of the getLeafJustAllocationDataWithFullyUnlockedCheck function.

In the _revokeDelegated function the totalEscrowed storage variable is decremented upon each iteration of the for-loop.

However it would be cheaper to cache the initial totalEscrowed value before beginning the loop and decrementing the cached value in the loop before finally re-assigning the totalEscrowed storage variable to the updated cached value.

Cache the initial totalEscrowed value before the for-loop, decrement the cached value on the stack in the for-loop, and assign the updated cached value to the storage variable after the for-loop.

When withdrawing, MerkleVester.withdraw function calls EscrowVester._reclaimFunds with 0 hardcoded for the amount out.

This will lead to all funds being undelegated from DelegationEscrow contract. If the amount being withdrawn is less than the total available, it is unnecessary to withdraw all of the tokens.

In some instances it is unnecessary to even call EscrowVester._reclaimFunds at all. This will force the user to call MerkleVester.delegateFunds again.

Check the vested amount prior to calling EscrowVester._reclaimFunds. Then verify if the user is required to pull out the entirety of their funds from DelegationEscrow contract.

In the EscrowVester contract the function selector blacklist does not allow benefactors to add additional selectors that are permanently blacklisted other than the ones hardcoded in the constructor.

Therefore if a benefactor wishes to use the MerkleVester escrow feature with a token that has a sensitive function that is not already included in the hardcoded blacklisted selectors, then the benefactor must assume some level of trust from the users that they will not whitelist the sensitive selector.

Consider implementing functionality such that the benefactor may add additional blacklisted selectors upon construction.

Declaring a memory variable inside a for loop is very gas intensive in Solidity. On each iteration a new memory variable is declared. The first 22 words of memory are priced linearly, but after that the price increases quadratically. This will significantly increase the chance of running out of gas in a transaction if the for loop iterates over a large array.

Declare the memory variable outside of the for loop and then reassign it in each iteration.

totalEscrow and escrowToken are private variables which will not have a getter function.

totalEscrow is kept track of in multiple locations costing gas from an sload and sstore. It currently tacks on unnecessary gas costs to transactions with no use case.

And escrowToken may be a useful queryable value.

Declare the totalEscrowed and escrowToken as public variables.

Events allow users to efficiently perform analysis of on-chain activity. Apps like Dune Analytics heavily rely on reading event logs. Adding events will allow users to gain insights on how the protocol is being utilized.

Add events for major state changing functions.

The adminWithdrawFeature is only assigned to in the constructor and cannot be updated, yet it is not declared as immutable.

Declare the adminWithdrawFeature as immutable.

The benefactor role carries a lot of admin privileges that can lead to loss of users funds or loss of functionality. For example, the benefactor can prevent further delegations even if the escrowFeature is enabled, via calling whitelistSelector with delegate(address) set to false. The benefactor should be a multisig wallet to increases chances that the account is not compromised.

Be sure to recommend that customers use a multisig wallet for the benefactor role. Furthermore, clearly document admin priveleges.

The EscrowVester whitelists certain functions to be called using their function selectors. However, multiple functions can produce the same selector, leading to a hash collision.

With the current whitelist, a user may pass arbitrary data to a token that doesn't call delegate but another function which happens to have the same selector, and lead to unexpected behavior.

Clearly document this risk to users.

Even after a benefactor calls function revokeAll, the beneficiary can still withdraw earned yield through withdrawEarned. This may be unexpected if the the benefactor intended for the revoke to return absolutely all funds.

Clearly document this behavior to users.

In the _withdrawToBeneficiary function the _validateWithdrawalInvariants function invocation passes the withdrawableAmount instead of the requestedWithdrawalAmount to verify the withdrawal invariants. This isn’t entirely accurate as the withdrawableAmount does not necessarily match how much is being withdrawn, however those is no veritable impact.

Perform the _validateWithdrawalInvariants check on the requestedWithdrawalAmount rather than the entire withdrawableAmount. Otherwise simplify the invariant check to only do a post check that involves the contract balance afterwards.

In the _withdrawInterval function the original _reclaimFunds call remains, while an additional _reclaimToFundWithdraw call has been added.

Remove the original _reclaimFunds call in the _withdrawInterval function.

The _reclaimToFundWithdraw function is inappropriately implemented such that reclaims will occur when the withdrawalAmount can be sufficiently withdrawn from the non-escrowed portion of the user's allocation.

Additionally, when the unclaimed amount is greater than the escrowed amount escrowed tokens will not be reclaimed when they ought to be.

The clearest example is an entirely vested allocation, where the escrowed portion will not be reclaimed at all, but the entire token amount will still be claimed from the contract's balance. This user no longer has an allocation to vest and so therefore should not garner any voting power from escrowed tokens.

Additionally, the fact that the existing _reclaimToFundWithdraw implementation passes the written tests indicates a lack of coverage for the new behavior.

Implement new written test cases which add coverage for the new expected behavior surrounding the reclaiming of funds for withdrawal.

Additionally, use the suggested _reclaimToFundWithdraw function:

function _reclaimToFundWithdraw(uint256 withdrawalAmount, Allocation memory allocation, uint256 withdrawn) internal {
    uint256 reclaim;

    // How much do we need to withdraw from escrow?
    uint256 leftToClaim = allocation.totalAllocation - withdrawn;
    uint256 allocationNotInEscrow = leftToClaim - escrowedFunds[allocation.id];

    if (allocationNotInEscrow < withdrawalAmount) {
      reclaim = withdrawalAmount - allocationNotInEscrow;
    }

    uint256 contractBalanceWithReclaim = IERC20(token).balanceOf(address(this)) + reclaim;

    if (contractBalanceWithReclaim < withdrawalAmount) {
      reclaim += withdrawalAmount - contractBalanceWithReclaim;
    }

    if (reclaim > 0) _reclaimFunds(reclaim, allocation.id);

  }

In the case that an allocation is cancelled partway through the vest, function defund is at risk of underflowing and preventing the terminated funds from being withdrawn. Consider the following example:

1. Bob creates 2 allocations for 50 tokens each, thus totalKnownObligations = 100.
2. Bob funds the Merkle contract for 100 tokens.
3. Alice and Chris delegate 50 tokens each, and merkle vester balance is now 0.
4. Bob cancels Alice's allocations halfway through, thus totalTerminated = 25.
5. contractBalance = 50 since 50 tokens are reclaimed from delegation escrow.
6. Bob calls defund which reverts since undefundable = 100 - 25 - 0 = 75 but contractBalance = 50 so defundable = 50 - 75 underflows.

Remove the defundable and undefundable logic. Afterwards, add a withdrawnFromTerminated variable, and then only allow the totalTerminated - withdrawnFromTerminated to be withdrawn.

When revokeAll() is invoked, it undelegates all users’ delegations and then transfers all tokens out of the vester smart contract.

If the Benefactor decides to create a new unlock schedule and fund the vester smart contract again, users from previous unlock schedules are still eligible to claim their tokens that were supposed to be revoked. This will lead to theft of tokens from users of the current unlock schedule.

Set schedules[allocationId].terminatedTimestamp to uint32(1) inside of _revokeAllDelegated(). This will prevent users from being able to withdraw and delegate revoked tokens, similarly to cancel() and revoke().

Additionally update the totalTerminated by the terminated amount in the revokeAll function.

Alternatively, consider removing the _revokeAllDelegated and revokeAll functions and simply require the benefactor to revoke vests one at a time.

If a contract is not fully funded for the entirety of an unlock schedule, only a portion of users will be able to delegate their funds. _validateEscrowRequest allows a user to delegate their total allocation minus what they have withdrawn, subtracted by the amount they are currently escrowing. If MerkleVester.sol is not funded for the entirety of an unlock schedule, user’s who delegate first, will be able to delegate an unfair proportion of tokens to users who delegated later.

Example:

Total allocation for all users: 1000 Alice’s allocation: 500 Bob’s allocation: 500 Contract balance: 500

Alice decides to delegate all of her allocation. The contract balance would be 0, since all of the funds will be in Alice’s delegation escrow. The transaction will pass the validations because Alice’s allocation is 500 and she has not withdrawn or escrowed yet. Now Bob is unable to delegate since all funds are stored in Alice’s delegation escrow.

Since the users’ total allocations are lazily stored, there is no efficient solution to verify that the contract is properly funded. Warn users to verify the contract is properly funded for the entirety of an unlock schedule. Additionally, we added a check inside our recommended revision for _reclaimToFundWithdraw to cover withdrawal debt obligations.

In the case that (requestedWithdrawalAmount > withdrawableAmount), the attempted withdraw reverts which is unexpected as right below the requestedWithdrawalAmount is adjusted to be no more than the available withdrawable: requestedWithdrawalAmount = _optionalOrMax(requestedWithdrawalAmount, withdrawableAmount);

Consequently, requested withdrawals that should be auto-adjusted and pass execution actually end up reverting.

Remove if (requestedWithdrawalAmount > withdrawableAmount) revert InsufficientFunds(); since _optionalOrMax is used to ensure this condition.

When an allocation is revoked or cancelled, all their escrowed funds are reclaimed and it is not possible to delegate those funds further. Recommendation given for H-02 should be combined with invariant checks for internal withdrawal functions so that revoked or cancelled allocations won't try reclaiming from escrow when there is no need to.

Change _withdrawInterval and _withdrawCalendar functions implementation with following diff in their respected places.

+        if (schedules[allocationId].terminatedTimestamp == 1) {
+            revert();
+        }

        uint256 unclaimedAmount = vestedAmount - schedule.withdrawn;

        uint256 requested = _optionalOrMax(withdrawalAmount, unclaimedAmount);

+        if (schedules[allocationId].terminatedTimestamp == 0) {
+            _reclaimToFundWithdraw(requested, allocation, allocationId);
+        }
-        _reclaimToFundWithdraw(requested, unclaimedAmount, allocationId);

Since totalKnownObligations is only updated when a user interacts with the protocol, calling cancel() or revoke() can cause totalTerminated to be greater than totalKnownObligations. This leads to a revert inside of defund() when calculating unDefundable: uint256 unDefundable = totalKnownObligations - totalTerminated - totalWithdrawn;. This will cause funds to be locked inside of the vester contract, until enough new users have interacted with the vester contract and can increase totalKnownObligations to be greater than totalTerminated.

Inside cancel() and revoke(), check if totalKnownObligations has been accounted for for the user. If totalKnownObligations has not been set, increase totalKnownObligations to prevent a revert in defund().