6.1 Releasing of vesting schedules in blocks could revert
VSophToken contract supports releasing of vesting schedules in blocks through releaseSchedulesInRange and releaseAllSchedules functions.
function releaseSchedulesInRange(uint256 startIndex, uint256 endIndex) external {
VestingSchedule[] storage schedules = vestingSchedules[msg.sender];
_releaseSchedulesInRange(schedules, startIndex, endIndex);
}
function releaseAllSchedules() external {
VestingSchedule[] storage schedules = vestingSchedules[msg.sender];
_releaseSchedulesInRange(schedules, 0, schedules.length);
}
These two functions accept a block of vesting schedules to be released in a single call. These two functions internally call the _releaseSchedulesInRange function based on start and end indexes qualified for the schedules to be processed for release. As the start dates for all the vesting schedules are not organised in chronological order, it is possible for one of the schedules to have a start date beyond the current block timestamp.
function _releaseSchedulesInRange(VestingSchedule[] storage schedules, uint256 startIndex, uint256 endIndex)
internal
{
if (schedules.length == 0) revert NoVestingSchedule();
if (startIndex >= endIndex || endIndex > schedules.length) revert InvalidRange();
uint256 totalAmountToRelease = 0;
uint256 vestingStartDate_ = vestingStartDate;
for (uint256 i = startIndex; i < endIndex; i++) {
==> uint256 released = _processSchedule(schedules[i], vestingStartDate_);
totalAmountToRelease += released;
}
if (totalAmountToRelease == 0) revert NoTokensToRelease();
}
So, while processing each schedule by calling _processSchedule, if the start time of the schedule happens to be a future time, meaning the vesting schedule did not start yet. In such a case, the transaction will revert, blocking all the other vesting schedules that were eligible to be released.
function _processSchedule(VestingSchedule storage schedule, uint256 vestingStartDate_)
internal
returns (uint256 releasedAmount)
{
// this is critical part. set startDate if schedule.startDate was zero
if (vestingStartDate_ != 0 && block.timestamp >= vestingStartDate_ && schedule.startDate == 0) {
schedule.startDate = vestingStartDate_;
}
==> if (!_hasVestingStarted(schedule)) revert VestingHasNotStartedYet();
//...
}
Recommendation
return 0 for vesting schedule where vesting has not yet started. This will allow release of tokens for other vesting schedules.
6.2 _transferAllVestingSchedules transfers even fully vested schedules
The _transferAllVestingSchedules function transfers all vesting schedules from one account to another, including those that are fully vested. The fully vested schedule does not have any pending amounts for future releases.
function _transferAllVestingSchedules(address from, address to) internal {
if (to == address(0) || from == address(0) || from == to) revert InvalidRecipientAddress();
if (vestingSchedules[from].length == 0) revert NoVestingSchedule();
VestingSchedule[] storage fromSchedules = vestingSchedules[from];
// Move vesting schedules
for (uint256 i = 0; i < fromSchedules.length; i++) {
vestingSchedules[to].push(fromSchedules[i]);
}
// --SNIP
Transferring fully vested schedules is redundant and unnecessary, as the tokens associated with them are no longer releasable. This leads to unnecessary growth of the vestingSchedules array for the recipient.
Recommendation
Modify the function to transfer only schedules that are not fully vested. This can be achieved by checking whether the schedule has any remaining releasable amount before transferring.
6.3 adminAddress should be validated for non zero address
In the initialize function, the adminAddress should be validated to be non-zero, as this account is entitled to many roles.
Recommendation
Validate adminAddress to be a non zero address.
