Description

According to the comments, the function removeMilestone should only remove milestones that have not yet been confirmed.

/// @notice removes a milestone from '_grantee''s MetaVesT if such milestone has not yet been confirmed, also making the corresponding 'milestoneAward' tokens withdrawable by controller

However, since there is no check on the confirmation status of milestones, it is also possible to remove milestones that have already been confirmed.

Impact

If a milestone is confirmed, its award will be added to milestoneAwardTotal as well as milestoneUnlockedTotal if unlockOnCompletion is true. Since the function removeMilestone does not handle these two variables, removing a confirmed milestone may cause accounting issues in MetaVesT.

function removeMilestone(uint256 _milestoneIndex) external onlyController {
    if(terminated) revert MetaVesT_AlreadyTerminated();
    if (_milestoneIndex >= milestones.length) revert MetaVesT_ZeroAmount();
    delete milestones[_milestoneIndex];
    emit MetaVesT_MilestoneRemoved(grantee, _milestoneIndex);
}

For example, in VestingAllocation, the function terminate will use milestonesAllocation, milestoneAwardTotal, and other variables to calculate the amount of tokens to recover. The milestonesAllocation is the sum of stored milestone awards. If a confirmed milestone is deleted, milestoneAwardTotal may be less than milestonesAllocation, causing the calculation to revert.

Recommendations

Consider implementing a confirmation-status check in BaseAllocation or MetaVesTController.

Remediation

This issue has been acknowledged by MetaLeX Labs, Inc, and a fix was implemented in commit 2c739437↗.