Treasury Manual Review Findings

TRE-01M: Insecure Management of Reserve & Liquidity Tokens

Description:

The registry arrays of reserve and liquidity tokens are improperly maintained which can cause huge discrepancies to the totalReserves measured in the auditReserves function. As an example, duplicate entries within a single array can cause a particular reserve to be calculated twice whereas an asset being a reserve and liquidity token at the same time will also cause an incorrect duplicate measurement of its balance value.

Example:

Copy
254/**
255 * @notice enable permission from queue
256 * @param _status STATUS
257 * @param _address address
258 * @param _calculator address
259 */
260function enable(
261    STATUS _status,
262    address _address,
263    address _calculator
264) external onlyOwner {
265    require(onChainGoverned, "OCG Not Enabled: Use queueTimelock");
266    if (_status == STATUS.SOHM) {
267        // 9
268        sOHM = IERC20(_address);
269    } else {
270        registry[_status].push(_address);
271        permissions[_status][_address] = true;
272
273        if (_status == STATUS.LIQUIDITYTOKEN) {
274            // 5
275            bondCalculator[_address] = _calculator;
276        }
277    }
278    emit Permissioned(_address, _status, true);
279}

Recommendation:

We advise all actions modifying those arrays to properly check for duplicates by preventing re-setting the same permission for an address to true.

Alleviation:

Both registry adjustment code blocks were updated to properly evaluate whether duplicate entries exist as well as to delete any previously set state in case the same token is being set between a liquidity and reserve token and vice versa.

TRE-02M: Weak Debt Position Validation

Description:

The incurDebt function mandates that the caller has a sufficient balance of sOHM to create the debt position, however, sOHM is a freely transferrable asset that should only be used as a data point when it cannot be transferred.

Example:

Copy
140/**
141    @notice allow approved address to borrow reserves
142    @param _amount uint
143    @param _token address
144 */
145function incurDebt(uint256 _amount, address _token) external override {
146    require(permissions[STATUS.DEBTOR][msg.sender], "Not approved");
147    require(permissions[STATUS.RESERVETOKEN][_token], "Not accepted");
148
149    uint256 value = tokenValue(_token, _amount);
150    require(value != 0);
151
152    uint256 availableDebt = sOHM.balanceOf(msg.sender).sub(debtorBalance[msg.sender]);
153    require(value <= availableDebt, "Exceeds debt limit");
154
155    debtorBalance[msg.sender] = debtorBalance[msg.sender].add(value);
156    totalDebt = totalDebt.add(value);
157
158    totalReserves = totalReserves.sub(value);
159
160    IERC20(_token).transfer(msg.sender, _amount);
161
162    emit CreateDebt(msg.sender, _token, _amount, value);
163}

Recommendation:

We advise the sOHM to either be held in custody or for some other similar mechanism to be put in place as the current debt mechanism is circumventable.

Alleviation:

The debt management system has now been built-in the sOHM implementation which in turn prevents transfers that would reduce the holder's balance below the required debt threshold.

TRE-03M: Improperly Valid Case

Description:

The tokenValue function should not yield a value of 0 under any circumstances as it will result in no-ops when utilized in mathematical operations and can cause the system to misbehave in case i.e. a token has more decimals than OHM is utilized in the evaluation that is not a LIQUIDITYTOKEN.

Example:

Copy
374/**
375    @notice returns OHM valuation of asset
376    @param _token address
377    @param _amount uint
378    @return value_ uint
379 */
380function tokenValue(address _token, uint256 _amount) public view override returns (uint256 value_) {
381    value_ = _amount.mul(10**IERC20Metadata(address(OHM)).decimals()).div(10**IERC20Metadata(_token).decimals());
382
383    if (permissions[STATUS.LIQUIDITYTOKEN][_token]) {
384        value_ = IBondingCalculator(bondCalculator[_token]).valuation(_token, _amount);
385    }
386}

Recommendation:

We strongly recommend the first linked require check to be relocated to the tokenValue function itself as no zero evaluations should be considered "valid".

Alleviation:

The Olympus DAO team considered this exhibit but decided to retain the current behaviour of the code in place.

TRE-04M: Inexistent Validation of Token Status

Description:

The manage function does not properly validate the permission status of the input _token which can improperly manipulate the totalReserves i.e. with no-op function implementations.

Example:

Copy
200/**
201    @notice allow approved address to withdraw assets
202    @param _token address
203    @param _amount uint
204 */
205function manage(address _token, uint256 _amount) external override {
206    if (permissions[STATUS.LIQUIDITYTOKEN][_token]) {
207        require(permissions[STATUS.LIQUIDITYMANAGER][msg.sender], "Not approved");
208    } else {
209        require(permissions[STATUS.RESERVEMANAGER][msg.sender], "Not approved");
210    }
211
212    uint256 value = tokenValue(_token, _amount);
213    require(value <= excessReserves(), "Insufficient reserves");
214
215    totalReserves = totalReserves.sub(value);
216
217    IERC20(_token).safeTransfer(msg.sender, _amount);
218
219    emit ReservesManaged(_token, _amount);
220}

Recommendation:

We advise the else branch of the first clause in manage to impose a require check ensuring that the permission of the _token specified is as a RESERVETOKEN.

Alleviation:

The totalReserves value is now adjusted solely when the input _token falls under either the liquidity or reserve token category, thereby alleviating this exhibit.

TRE-05M: Potentially Unsafe Primitive Evaluation

Description:

The evaluation of a particular token's value when the token is not a LIQUIDITYTOKEN is performed by a simple decimal-based conversion.

Example:

Copy
374/**
375    @notice returns OHM valuation of asset
376    @param _token address
377    @param _amount uint
378    @return value_ uint
379 */
380function tokenValue(address _token, uint256 _amount) public view override returns (uint256 value_) {
381    value_ = _amount.mul(10**IERC20Metadata(address(OHM)).decimals()).div(10**IERC20Metadata(_token).decimals());
382
383    if (permissions[STATUS.LIQUIDITYTOKEN][_token]) {
384        value_ = IBondingCalculator(bondCalculator[_token]).valuation(_token, _amount);
385    }
386}

Recommendation:

Apart from considering these tokens equal in value, it also directly relies on the presence of the decimals operator on the token which at times may not be available as it is an OPTIONAL member of the EIP-20 standard. We advise this particular trait to be considered carefully in the overall system as checks can be imposed at the inclusion level to prevent non-compliant tokens from being added.

Alleviation:

The Olympus DAO team considered this exhibit but decided to retain the current behaviour of the code in place.