Onyx Protocol security record

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.

In the contract `Swap.sol`, the role `Owner` has authority over the following functions: - withdraw() - setPrice() Any compromise to the `Owner` account may allow a hacker to take advantage of this authority and withdraw all the MDT tokens and modify the critical configuration in the contract.

Function `emergencyWithdraw()` does not trigger the `_moveDelegates()` function like the `withdraw()` function does.

The function `getPriorVotes()` contains a `require()` statement that the input `blockNumber` is smaller than the current block number. ```solidity=310 function getPriorVotes(uint256 _pid, address account, uint blockNumber) public view returns (uint256) { require(blockNumber < block.number, "Comp::getPriorVotes: not yet determined"); ... ``` However, in the `propose()` function in the `CHNGovernance` contract, the following lines of code call the `getPriorVotes()` function with `blockNumber` set as `block.number`, which would cause the aforementioned `require()` statement to fail. ```solidity function getVotingPower(address user, uint blockNumber) public view returns (uint256) { return chnTokenStaking.getPriorVotes(poolId, user, blockNumber); } function propose(address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas, string memory description) public returns (uint) { require(getVotingPower(msg.sender, block.number) >= proposalThreshold, "GovernorAlpha::propose: proposer votes below proposal threshold"); ... ```

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.

In the `add()` and `set()` functions, the flag `_withUpdate` determines if all the pools will be updated. This reliance might lead to significant loss of the reward. For illustration, assume we have only one pool with `pool.allocPoint == 50` and `totalAllocPoint == 50` at the beginning. Now we want to add another pool with `pool.allocPoint == 50`. There will be two scenarios on calculating the pool reward, Case 1: `_withUpdate` is set to `true`. - distribute the reward and update the pool. - add the given pool information Case 2: `_withUpdate` is set to `false`. - add the given pool information (Note: While we focused on the `add()` function, both the `add()` and `set()` functions update `totalAllocPoint`, which is used in calculation of pool rewards in the function `updatePool()` * In Case 1, reward for the first pool is updated in the call to `updatePool()` where `reward = multiplier.mul(rewardPerBlock).mul(pool.allocPoint).div(totalAllocPoint)`. * In Case 2, an update `totalAllocPoint = totalAllocPoint.add(_allocPoint)` is done first. Then `updatePool()` calculates the reward for the first pool: `reward = multiplier.mul(rewardPerBlock).mul(pool.allocPoint).div(totalAllocPoint);` Because the second pool is sharing rewards with the first one, the amount of reward for the first pool becomes half as much as that in the first case.

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()`.