TimedValuesStorage Manual Review Findings

TVS-01M: Potential Out-of-Bounds Value Read

Type	Severity	Location
Logical Fault		TimedValuesStorage.sol:L180-L201

Description:

The removeValue function will cause an out-of-gas failure in case a particular account does not have enough value to remove.

Example:

Copy
180/**
181 * @dev Removes given value from the TimedValue array
182 * @notice One by one, starting from the last one
183 */
184function removeValue(address account, uint256 value) external onlyOwner {
185    uint256 leftToRemove = value;
186
187    while (leftToRemove != 0) {
188        uint256 lastValue = _getLastValue(account);
189
190        if (leftToRemove >= lastValue) {
191            uint256 removed = _removeLastValue(account);
192            require(removed == lastValue, "removed value does not match");
193
194            leftToRemove -= lastValue;
195        }
196        else {
197            _decreaseLastValue(account, leftToRemove);
198            leftToRemove = 0;
199        }
200    }
201}

Recommendation:

We advise an explicit require check to be introduced instead that ensures there are more entries to remove from the account provided.

Alleviation:

The Colony Lab team considered this exhibit but opted not to apply a remediation for it in the current iteration of the codebase.

TVS-02M: Potentially Unsafe Length Assumption

Type	Severity	Location
Logical Fault		TimedValuesStorage.sol:L167-L177

Description:

The else execution path of pushValue assumes that the deposits array length exceeds the realDepositsLength of a particular account, however, if the opposite holds true the code will indirectly fail.

Example:

Copy
167if (realDepositsLength[account] == allocDepositLength(account)) {
168    deposits[account].push(timedValue);
169    realDepositsLength[account]++;
170
171}
172else {
173    // overwrite existing but removed value
174    uint256 firstFreeIdx = realDepositsLength[account];
175    deposits[account][firstFreeIdx] = timedValue;
176    realDepositsLength[account]++;
177}

Recommendation:

We advise an explicit require or assert check to be introduced instead ensuring that the allocDepositLength(account) is indeed greater than (>) the value of realDepositsLength[account].

Alleviation:

The Colony Lab team considered this exhibit but opted not to apply a remediation for it in the current iteration of the codebase.