Hifi Finance security record

The `HifiProxyTarget.depositUnderlying()` function is a simple wrapper function around the function `HToken.depositUnderlying()` which receives underlying tokens and sends back hTokens. The `HToken` contract will also internally increment the caller's balance. In this case, the caller will be the DSProxy contract. The `HToken.withdrawUnderlying()` function allows a user to withdraw the underlying tokens they deposited using the `HifiProxyTarget.depositUnderlying()` function. However, the `HifiProxyTarget` contract does not contain an equivalent for the `HifiProxyTarget.withdrawUnderlying()` function. The closes match in the `HifiProxy` contract would be the `HifiProxyTarget.removeLiquidityAndWithdrawUnderlying()` function, which would require a user to additionally add sufficient liquidity to the `HifiPool` contract. The user may not have the funds to add that liquidity, and as a result, they will not be able to withdraw their underlying tokens until maturity. At maturity, the user could call `HifiProxyTarget.redeem()`.

The `HToken.redeem()` function allows a user to redeem hTokens in exchange for underlying tokens after maturity. The `Fintroller` contract defines a configuration `isRedeemHTokenAllowed` for every `Bond` added which indicates whether the hToken can be redeemed. However, the `HToken.redeem()` currently allows a user to redeem hTokens even if `isRedeemHTokenAllowed` is false as defined by the `Fintroller`.

It is possible to call the configuration setter functions on Fintroller such that the overall configuration for a particular `Bond` becomes inconsistent or invalid. We have listed some of these possibilities below: 1. If borrowing is allowed for a `Bond`, then liquidation should also be allowed for a `Bond`. If liquidation is not allowed, it may allow the bad debt of the protocol to grow unreasonably. It is currently possible to set `isBorrowAllowed` for a Bond to `true` through the `setBorrowAllowed()` function, even when `isLiquidateBorrowAllowed` for the `Bond` is set to `false`. 2. The collateral ratio for a `Collateral` must always be greater than the liquidation incentive. For example, if the liquidation incentive is `10%`, then the minimum collateral ratio should be at least `110%`. However, no such check is made in `setCollateralRatio()`, leaving it possible to set a collateral ratio lower than a collateral's liquidation incentive. The same is true for `setLiquidationIncentive()`, which does not contain the necessary validation checks. 3. If liquidation is allowed for a `Bond`, then repayment should already be allowed. It would be unfair to allow a user's debt to be liquidated without giving them a chance to repay it. However, it is possible to call `setLiquidateBorrowAllowed()` such that liquidation is enabled while repayment is not enabled for a particular `Bond`. Furthermore, it is possible to disable liquidation for a `Bond` even when borrowing is allowed. As mentioned in point `1`, this should not be possible. 4. It should only be possible to disable repayment for a `Bond` if liquidation is also disabled. However, it is possible to disable repayment through the `setRepayBorrowAllowed()` function even when liquidation is still allowed for that `Bond.`

The Hifi protocol is highly dependent on Chainlink price feeds, which are read from the `ChainlinkOperator` contract. While the `getPrice()` function validates that the retrieved price exceeds zero, it does not validate the staleness of the price. Since an attacker could use the predictability of old prices to manipulate the protocol, it is critical that the protocol validates that all prices are fresh.

The ERC20 standard token implementation functions also returns the transaction status as a Boolean. It is good practice to check for the return status of the function call to ensure that the transaction was successful. It is the developer's responsibility to enclose these function calls with `require()` to ensure that when the intended ERC20 function call returns `false`, the caller transaction also fails. However, it is mostly missed by developers when they carry out checks; in effect, the transaction would always succeed, even if the token transfer didn't.

The `OwnableUpgradeable` contract is an `abstract` upgradeable contract. Contracts that inherit from `OwnableUpgradeable`, such as `BalanceSheetV2`, may introduce new storage variables in future versions. To allow for additional storage variables being added to future versions of `OwnableUpgradeable` without introducing storage collisions with `BalanceSheetV2`, a storage gap should be added to `OwnableUpgradeable`.

The `ChainlinkOperator.getPrice()` function uses an unsafe cast whereby an `int256` is directly cast to a `uint256`: `uint256 price = uint256(intPrice);`. This could result in an overflow if `intPrice` is negative. To resolve this issue, we recommend using the `toUint256()` function from OpenZeppelin's `SafeCast` library.