Locker Manual Review Findings

LOC-01M: Improper Invocation of EIP-20 transferFrom

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:

Copy
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

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:

Copy
65function deposit(
66    uint256 _amount,
67    uint256 _index,
68    bool force
69) 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: _amount
92            })
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

Description:

The contract's logic mandates that maxDeposits represents a non-zero value.

Example:

Copy
27constructor(
28    address _lockToken,
29    uint256 _lockTime,
30    uint256 _maxDeposits
31) 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

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:

Copy
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.