Ethereum Reserve Dollar (ERD)

`CollateralManager.priceUpdate()` iterates through the supported collaterals and fetches an updated price from their oracles. However, the for loop starts iterating from index `1`. Nothing in `addCollateral()` prevents any particular collateral type from occupying index `0`. Such a collateral would never have its price updated.

A liquidator can open a trove using a flashloan to deposit a large amount of collateral, increasing the number of rewards per unit staked to which they are entitled. In the same transaction they can trigger a liquidation and receive a portion of the redistributed rewards. They can then close their trove, also within the same transaction. This attack effectively steals a portion of the collateral rewards that should be going to existing trove holders. Note that the effectiveness of this attack increases as the size of the liquidation increases and as the amount of the collateral in the system decreases (so that they can more easily get a larger share).

When a user deposits stETH into a trove they recieve eTokens equivalent to the deposit amount. However, stETH rebases such that an address holding stETH will gradually receive more stETH over time. As of now, the codebase does not display any mechanism for allowing borrowers who collateralize stETH to claim these rewards. This will result in excess stETH accumulating in `ActivePool` and `DefaultPool`.

When liquidating a trove, one possibility is for the debt and collateral of that trove to be liquidated across all remaining active troves. This is done by adding the funds to the `DefaultPool` and applying the debt and collateral to an applicable trove (via `TroveManager.applyPendingRewards()`) when it is being adjusted, when it is being closed, and when it is being redeemed against. However, while debt is held in `DefaultPool`, interest is not accrued against it as it does not count towards the debt of an active trove. As a result, it would be in a borrower's interest to, with all else being equal, wait for as long as possible to adjust a trove or close it, as doing so would allow them to avoid paying interest on pending debt rewards.

Privileged roles are used extensively throughout the protocol for the purpose of configuration. However, we have identified a number of functions and roles that can lead to the loss of funds. * `USDEToken`: * `troveManagerAddress` and `borrowerOperationsAddress` can call `mintToTreasury()` and send mint arbitrary amounts of USDE to the `treasuryAddress`. * `stabilityPoolAddress` and `troveManagerLiquidationsAddress` can transfer any amount of USDE tokens between any addresses. * `ActivePool`: The `borrowerOperationsAddress`, `troveManagerAddress`, `stabilityPoolAddress`, `troveManagerRedemptionsAddress`, and `troveManagerLiquidationsAddress` can transfer all held collaterals to the `treasuryAddress` and `liquidityIncentiveAddress`. **All of these addresses can be set and changed by the contract owner.** * `DefaultPool`: The `troveManagerAddress` can transfer all held collaterals to the `activePoolAddress`. **All of these addresses can be set and changed by the contract owner.** * `LiquidityIncentive`: The owner has unrestricted access to transferring the contract's tokens. * `StabilityPool`: `borrowerOperations` has unlimited approval for the contract's `WETH`. **The owner can set and change the `borrowerOperations` address as well as the `WETH` address to another ERC-20 token.** * `Treasury`: The owner has unrestricted access to transferring the contract's tokens.

When a trove is inserted into `SortedTroves`, the ICR is recorded in the trove's corresponding `Node` struct as `Node.ICR`. However, as time passes, this recorded ICR is bound to become inaccurate with regard to the actual trove's ICR—the price of ETH is variable, and so is the amount of debt for which a trove is held responsible. Furthermore, the various collateral types are bound to fluctuate in price as well; notably, rETH is expected to increase in price with respect to time. Therefore, the sorting of the troves according to the stored ICR fields will change from that of a hypothetical sorted list that uses the actual ICR values. It is worth bearing in mind that there exists the `TroveManagerRedemptions.updateTroves()` function that allows the caller to update the trove sorting by reinserting specified troves—each reinsertion involves recording the trove's freshly updated ICR. However, this imposes a significant gas burden for the lifetime of the protocol on whatever parties take on the responsibility of the updating the trove sorting. Moreover, when reinserting the troves, while the ICR of the reinserted trove will be fresh, its location will depend on the potentially stale ICRs recorded in `SortedTroves`. Therefore, keeping the troves in the correct order may require repeatedly updating the ICRs of ALL the nodes in the list. In terms of protocol security, improper sorting of the troves poses a problem because trove sorting is the basis for the correct ordering of liquidations and redemptions: for as long as the sorting remains incorrect, troves are liable to redeemed against in the incorrect order—redemptions are made according to the trove ordering such that troves with lower ICRs (albeit above MCR) are redeemed against first—and liquidations that are made with the sequential liquidation function may liquidate troves in the wrong order (and lower ICR troves may be skipped over). It should be noted that documentation makes the following false statement as well: > Nodes also remain sorted as the ETH:USD price varies, since price fluctuations change the collateral value of each Trove by the same proportion.

The function `CollateralManager.removeCollateral()` removes a collateral type from the list of supported collaterals. Removal of a collateral type could result in a very sudden drop of users' ICRs. To take an extreme example, if a user has deposited only stETH and it becomes unsupported, their ICR would drop to zero. This would have a corresponding effect on TCR as well. The consequent effects may include multiple troves being put up for liquidation simultaneously as well as triggering of recovery mode.

The function `CollateralManager.removeCollateral()` removes a collateral type from the list of supported collaterals. Once no longer supported, users cannot withdraw their collateral in `BorrowerOperations.withdrawCollateral()`. Any users with a non-zero EToken balance would have no way of reclaiming their collateral. Additionally, if the EToken is changed by calling `CollateralManager.setEToken()`, the total supply of the previous EToken would map to irredeemable collateral. Furthermore, removal of a collateral type could result in a very sudden drop of users' ICRs. To take an extreme example, if a user has deposited only stETH and it becomes unsupported, their ICR would drop to zero. This would have a corresponding effect on TCR as well. Finally, if an EToken is transferred to an address without a trove, such users would be unable to redeem their collateral until they opened a trove.

Not checking return values can lead to unintended consequences, especially when the return value indicates the success of a process. Consider the following instances where return values are unchecked: * `BorrowerOperations.sol#257` * `BorrowerOperations.sol#723` * `BorrowerOperations.sol#869` * `BorrowerOperations.sol#871` * `CollateralManager.sol#249` * `CollateralManager.sol#618` * `StETHOracle.sol#276-282` * `StETHOracle.sol#285-301` * `StETHOracle.sol#314-331` * `PriceFeed.sol#613-628` * `PriceFeed.sol#637-643` * `PriceFeed.sol#646-662` * `PriceFeed.sol#674-691` * `StabilityPool.sol#276` * `StabilityPool.sol#1271` * `TroveManager.sol#392-396` * `TroveManager.sol#1270` * `TroveManagerRedemptions.sol#214` * `EToken.sol#104` * `USDEToken.sol#192`

All of the collateral in the protocol is stored between `ActivePool` and `DefaultPool`. Both contracts inherit from `OwnableUpgradeable`, which means that both can be upgraded by a user with a privileged role _immediately_. Moreover, both pools' funds can be accessed through other contracts inheriting from `OwnableUpgradeable`, which means if any of these other contracts' owners are compromised, they may also be able to siphon funds from the pools with great speed and ease. This is also possible through updating `EToken`, which is a token that can be used to redeem from troves. The last possibility is also applicable to `USDEToken`; if upgraded in a particular way (say, by letting an owner transfer the tokens indiscriminately), all the tokens can be stolen.