Manager Manual Review Findings

MAN-01M: Mismatch of Access Control

Type	Severity	Location
Logical Fault		Manager.sol:L261, L283

Description:

The setBalance function is meant to be invoked only by the administrator of the system yet the updateBalance function it internally invokes imposes a different set of access controls.

Example:

Copy
261function setBalance(SetTokenBalance[] calldata balances) external override onlyAdmin {
262    for (uint256 i = 0; i < balances.length; i++) {
263        SetTokenBalance calldata balance = balances[i];
264        updateBalance({
265            account: balance.account,
266            token: balance.token,
267            amount: balance.amount,
268            exchangeAmount: balance.exchangeAmount,
269            isPositive: balance.isPositive
270        });
271    }
272}
273
274function updateBalance(
275    address account,
276    address token,
277    uint256 amount,
278    uint256 exchangeAmount,
279    bool isPositive
280) public override {
281    require(token != address(0), "INVALID_TOKEN_ADDRESS");
282    require(account != address(0), "INVALID_ACCOUNT_ADDRESS");
283    require(msg.sender == voting || msg.sender == staking || pools.contains(msg.sender), "NOT_ALLOWED");
284
285    TokenBalance storage currentUserBalance = accountTokenBalances[account][token];
286    uint256 currentTotalBalance = totalTokenBalances[token];
287
288    uint256 updatedTotalBalance = currentTotalBalance.sub(currentUserBalance.amount).add(amount);
289    accountTokenBalances[account][token] = TokenBalance({token: token, amount: amount});
290    totalTokenBalances[token] = updatedTotalBalance;
291    emit BalanceUpdate(account, token, amount, exchangeAmount, isPositive);
292}

Recommendation:

We advise access control to be imposed atomically by refactoring the code to an internal function as currently the administrator call will fail if they are not within the pools entries.

Alleviation:

The code was refactored to instead check if the caller is an administrator and the conditional check for the staking address was omitted. We advise the Olive Protocol team to provide additional justification as to why the access control differs between those functions in this way.

MAN-02M: Improper Utilization of Duration

Type	Severity	Location
Logical Fault		Manager.sol:L148, L165

Description:

The variable cycleDuration is meant to represent the duration of each cycle, however, based on the bound checking performed by the contract the actual cycle duration is always equal to cycleDuration + 1 as a value of 0 would still require a single block to pass between cycles.

Example:

Copy
148require(block.number > (currentCycle.add(cycleDuration)), "PREMATURE_EXECUTION");

Recommendation:

We advise the comparisons to become inclusive and the value of cycleDuration to be enforced to be non-zero in its setter function and constructor to increase the accuracy and legibility of the codebase.

Alleviation:

The sanitizations advised have been introduced and the comparison was made inclusive but only for the first linked instance thereby partially alleviating this exhibit.

MAN-03M: Inexistent Rollover State Validation

Type	Severity	Location
Logical Fault		Manager.sol:L147, L212, L217

Description:

The linked functions should validate the value of rolloverStarted in the form of a require check however they do not, rendering the variable useless.

Example:

Copy
212function startCycleRollover() external override onlyRollover {
213    rolloverStarted = true;
214    emit CycleRolloverStarted(block.number);
215}
216
217function _completeRollover(string calldata rewardsIpfsHash) private {
218    currentCycle = block.number;
219    cycleRewardsHashes[currentCycleIndex] = rewardsIpfsHash;
220    currentCycleIndex = currentCycleIndex.add(1);
221    rolloverStarted = false;
222
223    if (voting != address(0)) {
224        IVoteOlive(voting).onCycleRollover(currentCycleIndex);
225    }
226
227    emit CycleRolloverComplete(block.number);
228}

Recommendation:

We advise proper state transitions to be enforced by introducing the corresponding require checks in the code.

Alleviation:

A require check has been introduced in both functions validating the value of rolloverStarted.

MAN-04M: Inexplicable Re-Invocation Capability

Type	Severity	Location
Logical Fault		Manager.sol:L116-L120, L122-L124

Description:

The linked functions adjust sensitive contract variables and can be executed an arbitrary number of times.

Example:

Copy
116function setVoting(address _voting) external override onlyAdmin {
117    require(_voting != address(0), "Cannot be zero address");
118    voting = _voting;
119    emit VotingSet(voting);
120}
121
122function setStaking(address _staking) external override onlyAdmin {
123    staking = _staking;
124}

Recommendation:

We advise them to be invoke-able only once by validating that the entry they are adjusting is equivalent to zero.

Alleviation:

The setStaking function has been omitted from the codebase whilst the setVoting function has been made as invoke-able only once thereby alleviating this exhibit.