Onyx Protocol - Audit

The OTokenEx contract is intended to be an ERC721 NFT token, but it does not conform to the EIP721 standard. The following issues are present: 1. No ownerOf() function: every NFT token must have an owner. 2. No setApprovalForAll()/isApprovedForAll()/getApproved() functions: ERC721 is different from ERC20. ERC721 does not have the concept of allowances - each token must be approved individually or an operator must be approved for all tokens owned by an owner. 3. transferFrom() is not properly implemented: it mistakenly uses the ERC20 allowance concept, and does not follow the ERC721 individual token or operator approval design. 4. Improper implementation of balanceOf(): the number of tokens owned by a user is the length of the array userTokens, not the value in accountTokens. For reference, see the ERC721 standard interface at https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v2.5.1/contracts/token/ERC721/IERC721.sol, and a proper implementation at https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v2.5.1/contracts/token/ERC721/ERC721.sol.

In the contracts `BaseJumpRateModelV2` and `DAIInterestRateModelV3` the role `owner` has authority over the function below. Any compromise to the `owner` account may allow the hacker to take advantage of this authority and update the parameters of the interest rate model. - updateJumpRateModel() In the contract `ChainlinkOracle` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority and set assets to any prices, set Chainlink aggregator as price feed for an asset, and transfer the `admin` role to a new account. - setDirectPrice() - setFeed() - setAdmin() - setUnderlyingPrice() In the contract `Comptroller` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority and set price oracle, set properties, change `marketCapGuardian` and `pauseGuardian`, pause OToken transfer, pause OToken minting, pause seizing assets, pause borrowing assets, and transfer XCN to the recipient. - _setPriceOracle() - _setCloseFactor() - _setCollateralFactor() - _setLiquidationIncentive() - _supportMarket() - _setMarketCaps() - _setMarketCapGuardian() - _setPauseGuardian() - _setMintPaused() - _setBorrowPaused() - _setTransferPaused() - _setSeizePaused() - _grantXcn() - _setXcnSpeeds() In the contracts `Comptroller` and `ComptrollerG5` the role `marketCapGuardian` has authority over the function below. Any compromise to the `marketCapGuardian` account may allow the hacker to take advantage of this authority and set market caps. - _setMarketCaps() In the contracts `Comptroller`, `ComptrollerG2`, `ComptrollerG3`, `ComptrollerG4`, and `ComptrollerG5`, the role `pauseGuardian` has authority over the functions below. Any compromise to the `pauseGuardian` account may allow the hacker to take advantage of this authority and pause OToken transfer, pause OToken minting, pause seizing assets, and pause borrowing assets. - _setMintPaused() - _setBorrowPaused() - _setTransferPaused() - _setSeizePaused() In the contract `ComptrollerG1` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _setPriceOracle() - _setCloseFactor() - _setCollateralFactor() - _setMaxAssets() - _setLiquidationIncentive() - _supportMarket() In the contract `ComptrollerG2` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _setPriceOracle() - _setCloseFactor() - _setCollateralFactor() - _setMaxAssets() - _setLiquidationIncentive() - _supportMarket() - _setPauseGuardian() - _setMintPaused() - _setBorrowPaused() - _setTransferPaused() - _setSeizePaused() In the contracts `ComptrollerG3`, `ComptrollerG4` and `ComptrollerG5` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _setPriceOracle() - _setCloseFactor() - _setCollateralFactor() - _setMaxAssets() - _setLiquidationIncentive() - _supportMarket() - _setPauseGuardian() - _setMintPaused() - _setBorrowPaused() - _setTransferPaused() - _setSeizePaused() - _setXcnRate() - _addXcnMarkets() - _dropXcnMarket() In the contracts `OToken` and `OTokenEx` the role `admin` has authority over the functions shown below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority and set properties, and reduce reserves by transferring to the admin. - _setPendingAdmin() - _setComptroller() - _setReserveFactor() - _reduceReserves() - _setInterestRateModel() In the contract `ODaiDelegate` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _becomeImplementation() - _resignImplementation() In the contract `OErc20` the role `admin` has authority over the functions shown below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority and withdraw ERC20 tokens except the underlying asset, and delegate the votes of the XCN-like underlying. - sweepToken() - _delegateXcnLikeTo() In the contracts `OErc20Delegator` and `OErc721Delegator` the role `admin` has authority over the function below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _setImplementation() In the contracts `Unitroller` the role `admin` has authority over the functions below. Any compromise to the `admin` account may allow the hacker to take advantage of this authority. - _setPendingImplementation() - _setPendingAdmin() - Function _become() of contracts `Comptroller`, `ComptrollerG1`, `ComptrollerG2`, `ComptrollerG3`, `ComptrollerG4` and `ComptrollerG5`

`21537311000 * 10 ** 18` tokens are sent to `msg.sender` when deploying the contract. This could be a centralization risk as the deployer can distribute tokens without obtaining the consensus of the community.

The functions `getAccountSnapshot()/balanceOfUnderlying()` uses the same `exchangeRate` to calculate the value of an owner's all tokens. But each erc721 token is unique and may have different prices.

NFT price can possibly be manipulated. For example, a person or a group of people can sell their nft tokens to themselves at artificially high prices. Even if chainlink nft floor price is used, there is still risk. Wash trading will mislead Onyx platform into believing that the fair market value of an NFT is the new floor price when in reality the price is not a result of natural demand. See https://blog.chain.link/what-is-an-nft-floor-price/

In the function `liquidateCalculateSeizeTokens()`, when calculating the number of collateral tokens to be seized, adding an extra repayment amount from the liquidation proxy leads to an increase in the number of seized collateral tokens for the liquidator. As we are aware from the proxy implementation address, the borrower is compensated with the borrowed tokens after deducting fees. However, liquidating with extra repay does not work properly in the following scenario. A borrower deposited two NFTs, NFT_1 and NFT_2, in order to borrow asset X. If the liquidator mistakenly uses this approach and repays assets with extra payments to liquidate NFT_1. The extra payments will be deposited to the market and mint oX tokens for the borrower. This means that the extra payments go to the borrower and the liquidator does not receive NFT_2, leading to a loss of extra payments for the liquidator. Liquidation proxy implementation address: https://goerli.etherscan.io/address/0x68d65662f93ec57fa8b805215e1690c8597e378d#contracts

The underlying token is an ERC721 token, and the EIP20 interface should not be used for ERC721 tokens, as the two types of tokens are fundamentally different.

In the aforementioned line, the formula for the calculation of `exchangeRate` is as follows after oToken is minted: $$ exchangeRate =\\ {totalCash + totalBorrows - totalReserves \over totalSupply } $$ ```solidity=340 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); } } ``` In solidity, division calculations have truncation problems. The `totalSupply` will be 1 and `exchangeRate` will be much smaller than `initialExchangeRate` in case the last user redeems (`accountTokens[redeemer] - 1`) oToken. As a result, the `exchangeRate` would be extremely small. When the value of `exchangeRate` is much smaller than `initialExchangeRate`, the user can mint oTokens well above normal values, and then the value of `exchangeRate` will be normal with the interest generating. In other words, the users can use this arbitrage to take away the underlying tokens in this pool. For example, the user can mint the amount of 1e8 OToken with one underlying token in case `exchangeRate = 1/1e8`.

The `require()` statements should provide a description of the condition not met.

The contract serves as the underlying entity to interact with `LiquidationProxy` and `CHNInterface` contracts. The scope of the audit treats contract that is out of scope as black boxes and assumes their functional correctness. However, in the real world, those contracts can be compromised.