Omniscia WallFair Audit
TokenLock Manual Review Findings
TokenLock Manual Review Findings
TLK-01M: Improper Staking Design
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Medium | TokenLock.sol:L73-L75 |
Description:
The vesting system imposed by TokenLock does not contain sane state expectations as it assigns the irreversible stake amounts prior to the actual transfer of tokens and does not validate that the tokens within the contract will ever be able to satisfy the vests.
Example:
72// create stakes, duplicates override each other and are not checked73for (uint256 ii = 0; ii < wallets_.length; ii += 1) {74 _stakes[wallets_[ii]].totalTokens = amounts_[ii];75}Recommendation:
We advise the totalTokens assignments to be set in a dedicated function that also invokes transferFrom on the token from the caller to ensure that the contract is properly funded to make the vest payouts.
Alleviation:
The system was restructured to instead contain multiple states via which the contract progresses to a validly funded state at which users are able to withdraw their stakes. The actual funding process does make sure the contract has sufficient tokens to pay out the vesting schedules thus alleviating this exhibit. We should note that the contract currently does not gracefully handle a surplus of tokens and we advise the fund function to assess whether extra tokens exist in the contract and refund them in some secure way to the original deployer of the contract.
TLK-02M: Inexistent Sanitization of Start Time
| Type | Severity | Location |
|---|---|---|
| Input Sanitization | Minor | TokenLock.sol:L67 |
Description:
The startTime_ variable should represent a time in the future.
Example:
40constructor(41 IERC20 token_,42 uint256 startTime_,43 uint256 vestingPeriod_,44 uint256 cliffPeriod_,45 uint256 initialReleaseFraction_,46 address[] memory wallets_,47 uint128[] memory amounts_48) {49 require(wallets_.length == amounts_.length, "number of elements in lists must match");50 // we put strong requirements for vesting parameters: this is not a generic vesting contract,51 // we support and test for just a limited range of parameters, see below52 require(vestingPeriod_ > 0, "vestingPeriod_ must be at least 30 days");53 // all periods must be divisible by 30 days54 require(vestingPeriod_ % DAYS_30_PERIOD == 0, "vestingPeriod_ must be divisible by 30 days");55 require(cliffPeriod_ % DAYS_30_PERIOD == 0, "cliffPeriod_ must be divisible by 30 days");56 // cliff must be shorted than total vesting period57 require(cliffPeriod_ < vestingPeriod_, "cliffPeriod_ must be less than vestingPeriod_");58 // decimal fraction is between 0 and 10**1859 require(initialReleaseFraction_ <= 10**18, "initialReleaseFraction_ must be in range <0, 10**18>");60 // cliff cannot be present if initial release is set61 require(62 !(initialReleaseFraction_ > 0 && cliffPeriod_ > 0),63 "cliff period and initial release cannot be set together"64 );65
66 _token = token_;67 _startTime = startTime_;68 _vestingPeriod = vestingPeriod_;69 _cliffPeriod = cliffPeriod_;70 _initialReleaseFraction = initialReleaseFraction_;71
72 // create stakes, duplicates override each other and are not checked73 for (uint256 ii = 0; ii < wallets_.length; ii += 1) {74 _stakes[wallets_[ii]].totalTokens = amounts_[ii];75 }76}Recommendation:
We advise such a check to be introduced to the codebase to prevent it from misbehaving.
Alleviation:
The Wallfair team stated that it is indeed a valid use case to have a start time that is before the actual deployment date of the contract, thus nullifying this exhibit.
TLK-03M: Misleading Error Message
| Type | Severity | Location |
|---|---|---|
| Input Sanitization | Minor | TokenLock.sol:L52 |
Description:
The error message's description does not match the check performed by it.
Example:
52require(vestingPeriod_ > 0, "vestingPeriod_ must be at least 30 days");Recommendation:
We advise the correct conditional or description to be used here as it potentially deviates from the specification.
Alleviation:
The error message was corrected to properly reflect the check it imposes.