Hifi Finance security record

The `Fintroller` contract defines a debt `ceiling` for every `Bond` added. The following code within the `BalanceSheetV2.borrow()` implies that the total debt is the supply of the hToken corresponding to the bond: ``` uint256 newTotalSupply = bond.totalSupply() + borrowAmount; uint256 debtCeiling = fintroller.getDebtCeiling(bond); if (newTotalSupply > debtCeiling) { revert BalanceSheet__DebtCeilingOverflow(newTotalSupply, debtCeiling); } ``` However, it is possible to increase the supply of the hToken past the debt ceiling by calling the `HToken.depositUnderlying()` function. This function mints new hTokens so long as the equivalent amount of underlying is supplied. There are no checks to ensure that hTokens will not be minted past the debt ceiling.

The `BalanceSheetV2` includes functions for repaying debt (`repayBorrow()`, `repayBorrowBehalf()`) as well as liquidating debt (`liquidateBorrow()`). All these functions will revert if the user attempts to repay more than the vault's current debt. The code below causes a revert when a user attempts to repay more than the vault's debt. Similar logic applies to liquidations. ``` uint256 debtAmount = vaults[borrower].debtAmounts[bond]; if (debtAmount < repayAmount) { revert BalanceSheet__RepayBorrowInsufficientDebt(bond, repayAmount, debtAmount); } ``` The logic above can lead to a denial of service for liquidators or borrowers trying to repay their loans. If a user tries to repay the total amount of their debt, they may be front-run by a malicious liquidator, which calls `repayBorrowBehalf()` with a minimal repayment. The transaction for the normal user will revert because the amount they are attempting to repay exceeds the vault's debt by a small amount. A similar scenario can occur when a malicious user prevents liquidation by front-running a valid call to `liquidateBorrow()` with either a small repayment or a small liquidation.

Each collateral has its own liquidation incentive, and the owner can change this value using the `setLiquidationIncentive()` function. The issue here is that before running the liquidation, a change request can be made by the owner to front-run the liquidation transaction. The worst-case scenario is that the user will receive less than what was expected.

The `HifiProxyTarget` features many functions which either add or remove liquidity from the `HifiPool` contract. All of these functions are missing sufficient protection against slippage. Liquidity providers are subject to impermanent loss when the interest rate for the `HifiPool` changes. The interest rate depends on the size of the hToken and underlying reserves. Liquidity providers must provide the underlying tokens and hTokens in their desired expected ratio. They should also always receive the amounts of tokens they expect when removing liquidity if they do not wish to realize an impermanent loss. There are some functions within the `HifiProxyTarget` which provide some slippage protection such as `addLiquidity()` which requires a `maxHTokenRequired` parameter. This protects the price of the hToken going up. However, there is no protection provided against the price of the underlying token (relative to the hToken) going up. To sufficiently protect against slippage, a `minHTokenRequired` parameter should be used as well. Similarly, functions that remove liquidity, such as `removeLiquidity()`, lack parameters to indicate the minimum amount of hTokens and underlying tokens received. We have listed all functions on the `HifiProxyTarget` which lack sufficient slippage protection: * `addLiquidity()` * `addLiquidityWithSignature()` * `borrowHTokenAndAddLiquidity()` * `borrowHTokenAndAddLiquidityWithSignature()` * `buyHTokenAndAddLiquidity()` * `buyHTokenAndAddLiquidityWithSignature()` * `buyUnderlyingAndAddLiquidity()` * `buyUnderlyingAndAddLiquidityWithSignature()` * `depositCollateralAndBorrowHTokenAndAddLiquidity()` * `depositCollateralAndBorrowHTokenAndAddLiquidityWithSignature()` * `depositUnderlyingAndMintHTokenAndAddLiquidity()` * `depositUnderlyingAndMintHTokenAndAddLiquidityWithSignature()` * `removeLiquidity()` * `removeLiquidityAndRedeem()` * `removeLiquidityAndRedeemWithSignature()` * `removeLiquidityAndSellHToken()` * `removeLiquidityAndSellHTokenWithSignature()` * `removeLiquidityAndWithdrawUnderlying()` * `removeLiquidityAndWithdrawUnderlyingWithSignature()` * `removeLiquidityWithSignature()`

The `getBurnOutputs()` function calculated the number of `hTokensReturned` using the following formula ``` hTokenReturned = (poolTokensBurned * hTokenReserves) / supply; ``` If the `poolTokensBurned * hTokenReserves` is less than `supply`, `hTokenReturned` will be equal to `0` due to a loss of precision.

The owner has the ability to add a new bond token using the `listBond()` function. This will create the new bond with the default parameters. However if the owner injected an existing bond he will override the old bond.

As a best practice and to prevent unwanted external contract side effects, it is advised to adhere to the ["Checks-Effects-Interactions"-pattern](https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html), even in the presence of [reentrancy guards](https://docs.openzeppelin.com/contracts/4.x/api/[](https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html)security#ReentrancyGuard). Following instances were observed where said pattern was violated: - `HifiProxyTarget.addLiquidity()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.borrowHTokenAndAddLiquidity()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.buyHToken()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.buyHTokenAndAddLiquidity()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.buyHTokenAndRepayBorrow()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.depositCollateral()`: Performs external contract calls (`collateral.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.depositUnderlyingAndMintHTokenAndAddLiquidity()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.removeLiquidity()`: Performs external contract calls (`hifiPool.underlying().safeTransfer()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.removeLiquidityAndRedeem()`: Performs external contract calls (`hToken.underlying().safeTransfer()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.removeLiquidityAndSellHToken()`: Performs external contract calls (`hifiPool.underlying().safeTransfer()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.sellUnderlyingAndRepayBorrow()`: Performs external contract calls (`underlying.transferFrom()`), before modifying state variables in following sub-calls. - `HifiProxyTarget.depositUnderlyingInternal()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls. - `HifiPool.sellUnderlying()`: Performs external contract calls (`underlying.safeTransferFrom()`), before modifying state variables in following sub-calls.

An early liquidity provider can manipulate a single share's price to raise it high enough such that only the largest providers can afford to add liquidity. The share price can be manipulated by first only adding a tiny amount of liquidity and then directly transferring a large amount of the underlying token to the pool.

The contract approves an address to transfer tokens on its behalf without setting a limit of how many tokens may be transferred (`token.approve(spender, type(uint256).max);`). If the approved address becomes hacked or is intentionally malicious, it may transfer out all the approved tokens.

Function `ChainlinkOperator.setFeed()` saves new feed information in storage variable `mapping(string => Feed) internal feeds;`, which is indexed by the assets symbol string. Trying to add different assets with the same symbol string would lead to collisions in said array, overwriting the existing asset.