Omniscia Flourishing Capital Audit

ERC20VestableInTimestamp Manual Review Findings

ERC20VestableInTimestamp Manual Review Findings

ERC-01M: Breaking EIP-20 Approval Adaptation

Description:

The EIP-20 standard denotes that an approve function invocation should succeed even if the approver does not have the necessary balance at that point in time. This is a pattern commonly applied by DeFi strategies which pre-approve DeFi contracts with an unlimited approval (type(uint256).max) to ensure that the target contracts will be able to at all times extract funds from the contract. The onlyIfFundsAvailableNow modifier applied to the approve condition disallows this and instead only allows an approval to be set at maxium equal to the balance of the user, causing a lot of contracts in the EVM ecosystem to break / misbehave.

Example:

contracts/ERC20VestableInTimestamp.sol
505function approve(address spender, uint256 value)
506    public
507    override
508    onlyIfFundsAvailableNow(_msgSender(), value)
509    returns (bool)
510{
511    return super.approve(spender, value);
512}

Recommendation:

We advise the modifier to be omitted from the approve function as it is applied in the transferFrom invocation in any case.

Alleviation:

The function is now properly not overridden.

ERC-02M: Incorrect Schedule Retrieval

Description:

The getAllSchedulesOfBeneficiary function incorrectly filters the active schedules of a user as it assumes that the active schedules are in sequence.

Example:

contracts/ERC20VestableInTimestamp.sol
251function getAllSchedulesOfBeneficiary(address _beneficiary)
252    public
253    view
254    returns (uint256[] memory userActiveSchedules)
255{
256    uint256 index = 0;
257    uint256[] memory schedules = new uint256[](allActiveSchedules.length);
258    for (uint256 i = 0; i < allActiveSchedules.length; i++) {
259        if (
260            userVestingAmountInSchedule[_beneficiary][
261                allActiveSchedules[i]
262            ] > 0
263        ) {
264            schedules[index] = allActiveSchedules[i];
265            index++;
266        }
267    }
268

269    uint256 activeCount = 0;
270    for (uint256 i = 0; i < schedules.length; i++) {
271        if (schedules[i] > 0) activeCount++;
272    }
273    userActiveSchedules = new uint256[](activeCount);
274    for (uint256 i = 0; i < activeCount; i++) {
275        userActiveSchedules[i] = schedules[i];
276    }
277}

Recommendation:

We advise the last for loop to be substituted by a while loop that evaluates that activeCount is non-zero and we advise activeCount to be subtracted each time a non-zero schedule is identified and added to the userActiveSchedules which may not be in sequence.

Alleviation:

The logic of the function now properly uses a seperate indexor for the schedules array that is incremented only when valid entries are found, leading to the latter of the two for loops iterating over correct values.

ERC-03M: Inexplicable Reset Potential

Description:

The setEndTimeLock function can be invoked an arbitrary of times thus locking the token at any point in time, potentially perpetually.

Example:

contracts/ERC20VestableInTimestamp.sol
132function setEndTimeLock(uint256 _endTimeLock) public onlyGrantor {
133    require(_endTimeLock > block.timestamp, "end lock time in the past");
134    endTimeLock = _endTimeLock;
135}

Recommendation:

We advise such functionality to be removed from the codebase by ensuring that the endTimeLock value is set only once.

Alleviation:

The codebase has been revamped and as such, the finding is no longer applicable.

ERC-04M: Deviation of Specification

Description:

The _applyVestingSchedule function indicates that an ID of 0 should not be invalid for a particular schedule, however, this value is permitted in the updateVestingSchedule function.

Example:

contracts/ERC20VestableInTimestamp.sol
137// Update the information of a Vesting Schedule
138function updateVestingSchedule(
139    string memory _name,
140    uint256 _id,
141    bool _isActive,
142    uint256 _startDay,
143    uint256 _cliffDuration,
144    uint256 _duration
145) public onlyGrantor {
146    // Chech for a valid vesting schedule give (disallow absurd values to reject likely bad input).
147    require(
148        _duration > 0 &&
149            _duration <= TEN_YEARS_SECONDS &&
150            _cliffDuration < _duration,
151        "invalid vesting schedule"
152    );
153

154    require(
155        _startDay >= JAN_1_2000 && _startDay < JAN_1_3000,
156        "invalid start day"
157    );
158

159    VestingSchedule storage vestingSchedule = vestingSchedules[_id];
160    if (vestingSchedule.isActive && !_isActive) {
161        inActiveSchedule(_id);
162        scheduleQuantity = scheduleQuantity - 1;
163    } else if (!vestingSchedule.isActive && _isActive) {
164        allActiveSchedules.push(_id);
165        scheduleQuantity = scheduleQuantity + 1;
166    }
167    vestingSchedule.scheduleName = _name;
168    vestingSchedule.isActive = _isActive;
169    vestingSchedule.startDay = _startDay;
170    vestingSchedule.cliffDuration = _cliffDuration;
171    vestingSchedule.duration = _duration;
172

173    emit VestingScheduleUpdated(
174        _id,
175        _name,
176        _isActive,
177        _startDay,
178        _cliffDuration,
179        _duration
180    );
181}
182

183/**
184 * @dev This operation permanently establishes the vesting schedule in the beneficiary's account.
185 *
186 * @param _beneficiary = Address which will be set the vesting schedule on it.
187 * @param _vestingSchedule = Vesting schedule ID.
188 * @param _vestingAmount = The amount of token will be vested.
189 */
190function _applyVestingSchedule(
191    address _beneficiary,
192    uint256 _vestingSchedule,
193    uint256 _vestingAmount
194) internal {
195    require(_vestingSchedule != 0, "invalid vesting schedule type");
196    require(userVestingAmountInSchedule[_beneficiary][_vestingSchedule] == 0, "already applied vesting schedule");
197    require(
198        vestingSchedules[_vestingSchedule].isActive == true,
199        "vesting schedule is not active"
200    );
201    userVestingAmountInSchedule[_beneficiary][
202        _vestingSchedule
203    ] = _vestingAmount;
204}

Recommendation:

We advise the same requirement to be enforced in the updateVestingSchedule function to ensure the codebase complies with its specification.

Alleviation:

The logic was refactored to no longer conduct any validations within that function considering this exhibit null.

ERC-05M: Inexistent Overriddance of Burn Functionality

Description:

The ERC20VestableInTimestamp token inherits from the ERC20Burnable implementation which exposes the burn and burnFrom functions that are freely invoke-able at any time of the contract's lifetime, going against its specification within the transfer and transferFrom functions.

Example:

contracts/ERC20VestableInTimestamp.sol
5import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
6import "@openzeppelin/contracts/access/AccessControl.sol";
7

8abstract contract ERC20VestableInTimestamp is ERC20Burnable, AccessControl {

Recommendation:

We advise these functions to also be overridden and disallow tokens to be burned before they are vested to comply with the specification of the contract.

Alleviation:

Both the burn and burnFrom functions are now correctly overridden and contain the corresponding modifier hook to validate that the parties are able to "transact" the amount to be burned.

ERC-06M: Ungraceful Vesting Error Handling

Description:

The applyMultiVestingSchedule function iterates through three arrays meant to represent what vesting schedule should be applied to which user and for how many units, however, the iteration will fatally fail reverting all state changes if only one of the items in the array does not satisfy the _applyVestingSchedule pre-conditions.

Example:

contracts/ERC20VestableInTimestamp.sol
183/**
184 * @dev This operation permanently establishes the vesting schedule in the beneficiary's account.
185 *
186 * @param _beneficiary = Address which will be set the vesting schedule on it.
187 * @param _vestingSchedule = Vesting schedule ID.
188 * @param _vestingAmount = The amount of token will be vested.
189 */
190function _applyVestingSchedule(
191    address _beneficiary,
192    uint256 _vestingSchedule,
193    uint256 _vestingAmount
194) internal {
195    require(_vestingSchedule != 0, "invalid vesting schedule type");
196    require(userVestingAmountInSchedule[_beneficiary][_vestingSchedule] == 0, "already applied vesting schedule");
197    require(
198        vestingSchedules[_vestingSchedule].isActive == true,
199        "vesting schedule is not active"
200    );
201    userVestingAmountInSchedule[_beneficiary][
202        _vestingSchedule
203    ] = _vestingAmount;
204}
205

206/**
207 * @dev Immediately set multi vesting schedule to an address, the token in their wallet will vest over time
208 * according to this schedule.
209 *
210 * @param _beneficiaries = Addresses to which tokens will be vested.
211 * @param _vestingSchedules = Vesting schedule IDs.
212 * @param _vestingAmounts = The amount of tokens that will be vested.
213 */
214function applyMultiVestingSchedule(
215    address[] calldata _beneficiaries,
216    uint256[] calldata _vestingSchedules,
217    uint256[] calldata _vestingAmounts
218) public onlyGrantor returns (bool ok) {
219    require(_beneficiaries.length == _vestingSchedules.length, "invalid schedules length");
220    require(_vestingSchedules.length == _vestingAmounts.length, "invalid amounts length");
221

222    for (uint256 i = 0; i < _beneficiaries.length; i++) {
223        _applyVestingSchedule(
224            _beneficiaries[i],
225            _vestingSchedules[i],
226            _vestingAmounts[i]
227        );
228    }
229

230    return true;
231}

Recommendation:

Given that the vesting schedule that should be applied to each member is independent as indicated by the _vestingSchedules being specified in an array as well, we advise the loop to gracefully handle such errors either by halting the loop the time the error is detected or simply noting it and continuing iteration.

Alleviation:

The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase.

ERC-07M: Ungraceful Whitelist Error Handling

Description:

The setWhitelist function does not apply graceful error handling in case the status of an input address is being re-set to the same, causing the whole transaction to fail.

Example:

contracts/ERC20VestableInTimestamp.sol
117function setWhiteList(address[] memory _whiteListAddresses, bool _status)
118    public
119    onlyGrantor
120{
121    require(_whiteListAddresses.length > 0, "invalid length");
122    for (uint256 i = 0; i < _whiteListAddresses.length; i++) {
123        require(
124            whiteLists[_whiteListAddresses[i]] != _status,
125            "whitelist status is not change"
126        );
127        whiteLists[_whiteListAddresses[i]] = _status;
128    }
129}

Recommendation:

We advise the require check to be omitted as it currently would cause a transaction of 20 account status sets to fail if the last one was being re-set.

Alleviation:

The require check was omitted from the codebase and instead an array is now passed in place of the _status argument thus allowing it to set each whitelist address to a different status.

Prev

ERC20VestableInTimestamp.sol (ERC-S)

Next

FlourishingAIToken.sol (FAI-M)

On this page