Omniscia Vector Finance Audit
Locker Manual Review Findings
Locker Manual Review Findings
LOC-01M: Improper Invocation of EIP-20 transferFrom
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | Locker.sol:L71-L73 |
Description:
The linked statement does not properly validate the returned bool
of the EIP-20 standard transferFrom
function. As the standard dictates, callers must not assume that false
is never returned, however, certain tokens such as USDT (Tether) are non-standard and yield no bool
causing such checks to fail.
Example:
71require(72 IERC20(lockToken).transferFrom(msg.sender, address(this), _amount)73);
Recommendation:
We advise a safe wrapper library to be utilized instead such as SafeERC20
by OpenZeppelin to opportunistically validate the returned bool
only if it exists.
Alleviation:
The code was refactored to no longer deal with asset transfers rendering this exhibit null.
LOC-02M: Improper Usage of Indexes
Type | Severity | Location |
---|---|---|
Input Sanitization | Minor | Locker.sol:L67 |
Description:
The deposit
function is meant to permit a user to deposit using an index-based system, however, in the execution flow of a new deposit an entry is simply pushed to the userDeposits
array and does not necessarily represent the _index
passed in to the contract, causing misleading events to be emitted it.
Example:
65function deposit(66 uint256 _amount,67 uint256 _index,68 bool force69) external {70 require(_index < maxDeposits, "Not in the range of allowed deposits");71 require(72 IERC20(lockToken).transferFrom(msg.sender, address(this), _amount)73 );74 _mint(msg.sender, _amount);75 if (_index < userDeposits[msg.sender].length) {76 userDeposit storage depositUser = userDeposits[msg.sender][_index];77 if (depositUser.amount > 0) {78 require(79 force,80 "Deposit will extend lock time of previous, launch with force=True"81 );82 }83 depositUser.depositTime = block.timestamp;84 depositUser.endTime = block.timestamp + lockTime;85 depositUser.amount = depositUser.amount + _amount;86 } else {87 userDeposits[msg.sender].push(88 userDeposit({89 depositTime: block.timestamp,90 endTime: block.timestamp + lockTime,91 amount: _amount92 })93 );94 }95 emit NewDeposit(msg.sender, _amount, _index);96}
Recommendation:
We advise the _index
member to be properly sanitized by either ensuring it equals the length
member of userDeposits
or a special value used for new entries such as the maximum of uint256
.
Alleviation:
The index is now mandated to be within a particular range the comparison of which guarantees the else
branch's execution is only performed when the _index
is equivalent to userDeposits[user].length
thus alleviating this exhibit.
LOC-03M: Inexistent Sanitization of Max Deposits
Type | Severity | Location |
---|---|---|
Input Sanitization | Minor | Locker.sol:L34 |
Description:
The contract's logic mandates that maxDeposits
represents a non-zero value.
Example:
27constructor(28 address _lockToken,29 uint256 _lockTime,30 uint256 _maxDeposits31) baseERC20("Locked VTX", "LVTX") {32 lockToken = _lockToken;33 lockTime = _lockTime;34 maxDeposits = _maxDeposits;35}
Recommendation:
We advise such a constraint to be imposed by the code itself via a corresponding require
check.
Alleviation:
The value of maximum deposits is now guaranteed to be a positive number.
LOC-04M: Inexistent Restriction of Token Flow
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | Locker.sol:L9, L98-L107 |
Description:
The Locker
token contract is meant to represent a token that is consequently burned to unlock the underlying staked deposit, however, the token is freely transactable.
Example:
98function withdraw(uint256 _amount, uint256 _index) external {99 require(_index < maxDeposits, "Not in the range of allowed deposits");100 _burn(msg.sender, _amount);101 userDeposit storage userDepositInfo = userDeposits[msg.sender][_index];102 require(userDepositInfo.amount >= _amount, "Not enough deposit");103 require(userDepositInfo.endTime <= block.timestamp, "Not unlocked");104 userDepositInfo.amount -= _amount;105 require(IERC20(lockToken).transfer(msg.sender, _amount));106 emit NewWithdraw(msg.sender, _amount, _index);107}
Recommendation:
We advise transference of the token to be restricted by overriding the corresponding transfer
and transferFrom
functions in the baseERC20
implementation as otherwise a user can incorrectly transfer their tokens and thus be unable to claim their locked deposit. Alternatively, we advise a lock transference mechanism to be implemented that transfers the locks associated with a particular batch of tokens as well.
Alleviation:
The code was refactored to only allow deposit and withdrawal interaction from the masterchief
address with no actual funds transferred in and out of the contract. Additionally, transfer capabilities are now prohibited by the contract via a false
return on the transfer
and transferFrom
functions. Although the exhibit is dealt with due to the revised deposit and withdrawal concepts, we advise the Vector team to prevent execution of transfer
and transferFrom
altogether by executing a revert
statement within the function bodies.