Onyx Protocol - Audit

The `_addReserves()` function accrues interest and adds reserves by transferring assets from the sender. The formula for the calculation of `exchangeRate` is as follows: $$ exchangeRate = {totalCash + totalBorrows - totalReserves \over totalSupply } $$ ```solidity=342 function exchangeRateStoredInternal() internal view returns (MathError, uint) { uint _totalSupply = totalSupply; if (_totalSupply == 0) { /* * If there are no tokens minted: * exchangeRate = initialExchangeRate */ return (MathError.NO_ERROR, initialExchangeRateMantissa); } else { /* * Otherwise: * exchangeRate = (totalCash + totalBorrows - totalReserves) / totalSupply */ uint totalCash = getCashPrior(); uint cashPlusBorrowsMinusReserves; Exp memory exchangeRate; MathError mathErr; (mathErr, cashPlusBorrowsMinusReserves) = addThenSubUInt(totalCash, totalBorrows, totalReserves); if (mathErr != MathError.NO_ERROR) { return (mathErr, 0); } (mathErr, exchangeRate) = getExp(cashPlusBorrowsMinusReserves, _totalSupply); if (mathErr != MathError.NO_ERROR) { return (mathErr, 0); } return (MathError.NO_ERROR, exchangeRate.mantissa); } } ``` The `getCashPrior()` function returns `totalSupply`, the NFT can not be borrowed, so the `totalBorrows` is always 0. Based on the logic of functions `mintFresh()` and `redeemFreshOToken()`, it can be inferred that the exchange rate between the underlying asset and the token is 1:1. If the `_addReserves()` function is not disabled, the exchange rate will decrease, the return value `redeemAmount` at line 633 in the `OTokenEx` contract may be 0 when the sender redeems underlying asset using the function `redeem()`/`redeems()`. Worse, if `totalReserves` equals `totalCash`, the exchange rate will be 0 and the sender will not be able to mint/redeem.

The input parameter `tokenIndex` is not checked for a valid array index. If the `tokenIndex` is not correct, the `doTransferOut()` function will revert and the user can not redeem the underlying asset.

The functions `setUnderlyingPrice()` and `setDirectPrice()` can be called by anyone as it has no access restriction. This enables anyone to call this and update the `prices` configuration.

The following codes in the function `redeemFresh()/borrowFresh()` do not meet the Checks-Effects-Interactions pattern. ```solidity=787 doTransferOut(borrower, borrowAmount); /* We write the previously calculated values into storage */ accountBorrows[borrower].principal = vars.accountBorrowsNew; accountBorrows[borrower].interestIndex = borrowIndex; totalBorrows = vars.totalBorrowsNew; ``` It only has a reentrancy lock as there is no lock at the Controller level, only the OToken level. If the `OToken` is an ERC777/ERC677 protocol, the reentrancy can happen in function levels of an ERC777/ERC677-based contract, i.e. multiple function calls that are triggered by the hook mechanism of ERC777/ERC677. This issue is possible to happen with all compound forks, but Compound is not affected as they do not list tokens with callback functionality.

The `_reduceReservesFresh()` function reduces reserves by transferring to admin. However, the second argument of `doTransferOut()` function is the index of the token, not the reduction amount. Additionally, the `doTransferOut()` function can only transfer one NFT at a time. As a result, the reduction amount should always be 1, and the admin can only transfer their own tokens.

The `claimXcn()` function of the `Comptroller` contract requires that the token must be listed. However, the `pendingXcn()` function does not exclude the unlisted token. The same issue exists with the function `getDailyXCN()`.

In the `distributeSupplierXcn()` function of the `Comptroller` contract, line 1276 covers the case where users supplied tokens before the market's supply state index was set. Rewards the user with `xcnInitialIndex` accrued from the start of when supplier rewards were first set for the market. So the `distributeSupplierXcn()` function of the `OnyxLens` contract should use the same comparison conditions.

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=51 admin = admin_; ``` - `admin_` is not zero-checked before being used. --- ```solidity=68 implementation = implementation_; ``` - `implementation_` is not zero-checked before being used. --- ```solidity=52 admin = admin_; ``` - `admin_` is not zero-checked before being used. --- ```solidity=69 implementation = implementation_; ``` - `implementation_` is not zero-checked before being used. --- ```solidity=36 target = target_; ``` - `target_` is not zero-checked before being used. --- ```solidity=415 MINT_BASE_TOKEN = mintBaseToken; ``` - `mintBaseToken` is not zero-checked before being used.

In the `oErc721` contract, the sender cannot borrow the assets, so the aforementioned lines are redundant since the `totalBorrows` is always 0.

In the `Comptroller` contract, the function `getXcnAddress()` returns the address `0xc03Fa5e041BE9C8deD030D6dec7936cE67D78ae8` as the XCN token address. However, in the `ComptrollerG3`, `ComptrollerG4`, and `ComptrollerG5` contracts, this function returns the address `0xA2cd3D43c775978A96BdBf12d733D5A1ED94fb18`.