RaymondFam-Analysis.md

The following is a detailed analysis of the Reserve Protocol, with a comprehensive exploration of the codebase, architecture, quality, centralization risks, mechanisms, and potential systemic risks. In this report, six key vulnerabilities have been identified and ranked by severity, with three identified as high, and the remaining three as medium. The potential issues range from token loss during transfers to early exploits, reentrancy vulnerabilities, incorrect collateral pricing, and lack of protection during withdrawals. Each vulnerability is thoroughly explained with corresponding recommendations to mitigate the associated risks. The report further covers the evaluation approach, providing insightful suggestions for improving the codebase and minimizing centralization risks. The mechanism review and an assessment of systemic risks offer an understanding of the potential threats that need to be managed for long-term system stability.

Comments for the judge to contextualize my findings

A total of 3 highs, 3 mediums along with additional inputs in the Gas and QA reports have been submitted. It's worth noting that loopholes still abound depending on how you would visualize the codebase both short and long terms. Here is the breakdown of the HM findings in their condensed forms:

1. Potential Loss of Rewards During Token Transfers in StaticATokenLM.sol (High) The report identifies a potential issue in the Reserve Protocol, where the process of transferring tokens could lead to permanent loss of rewards for the sender due to the order of function calls and behavior of certain functions. The issue arises primarily from the _getPendingRewards function not accounting for all recent rewards from the INCENTIVES_CONTROLLER when the fresh parameter is set to false. This miscalculation could then negatively affect the output of the _updateUserSnapshotRewardsPerToken function, resulting in some rewards being missed, particularly affecting the Backing Manager during RToken redemptions. The issue could also affect token transfers to successful auction bidders. The report recommends mitigating this by introducing an additional call to update the state of rewards before any token transfer occurs, specifically within the _beforeTokenTransfer function, to ensure accurate internal accounting of rewards and prevent loss or premature gain during transfers.
2. Potential Early Exploit in Morho-Aave ERC4626 Implementation (High) The report describes an exploit in a blockchain smart contract where an attacker could potentially gain unauthorized control over a significant portion of the assets stored within the contract's "vault". This is possible when the vault is initially empty, and an attacker deposits a negligible amount before a legitimate user's deposit, thereby owning shares while the total vault asset is still low. The attacker then donates a large amount to the vault, causing a precision error which leads to other users receiving no shares despite depositing assets. The attacker redeems their shares, possibly gaining a significant portion of the vault's assets. This vulnerability arises from the flawed inheritance structure of the underlying smart contract and its functions, particularly the MorphoTokenisedDeposit. _decimalsOffset() function. A proof-of-concept is given which shows how an attacker can implement this exploit in five steps. The report recommends mitigating this risk by hardcoding MorphoTokenisedDeposit. _decimalsOffset() to return 9 instead of 0.
3. Cross-Function Reentrancy Vulnerability Leading to Unintended Token Minting in RewardableERC20Wrapper.deposit (High) The report outlines a substantial reentrancy vulnerability within the RewardableERC20Wrapper contract, particularly the deposit() function. This vulnerability, which can be exploited through direct and cross-function reentrancy attacks, becomes evident when ERC777 tokens or other token types with "hook" features serve as the underlying token. Successful exploitation could trigger unwarranted token minting, leading to a skewed token supply, thereby compromising the contract's integrity and impacting individual token value. This susceptibility is due to the _mint(_to, _amount) operation execution prior to the underlying.safeTransferFrom(msg.sender, address(this), _amount) call within the deposit() function. Such a sequence allows an ERC777 token to initiate a reentrant call back to deposit(), thereby allowing token minting before the contract's state is adequately updated. To mitigate this, the report suggests implementing a reentrancy guard for all public and external functions that modify the contract's state and call external contracts. It also recommends refactoring the deposit() function to adhere to the check-effects-interactions pattern, conducting state changes post external calls.
4. Risk of Incorrect Collateral Pricing in Case of Aggregator Reaching minAnswer (Medium) The report highlights a significant issue within Chainlink aggregators related to the built-in circuit breaker, which can cause the oracle to continuously return the minPrice rather than the actual asset price during substantial price drops. The problem occurs when an asset's price goes below its minPrice; the protocol then overvalues the token at the minPrice, leading to overvalued function calls. The issue becomes prominent when the defaultThreshold is set in a way that keeps the pegPrice between pegBottom and pegTop, which can potentially lead to an excessively large issuance of RTokens, creating an unnoticed unhealthy collateral basket. Although a combination of oracles, such as Chainlink and Band, could potentially prevent this situation, a malicious user could still exploit Band by DDOSing relayers to block price updates. To address this issue, the report suggests cross-checking the returned price in the OracleLib.price against minPrice/maxPrice, and triggering a revert if the price falls outside these boundaries, thus ensuring an accurate price representation.
5. Lack of protection when withdrawing Static Atoken (Medium) The report reveals a significant issue with the Aave plugin regarding its ever-increasing exchange rate. It shows that while an early wrap of AToken results in more Static Atoken minting, withdrawal timing can greatly impact the amount of AToken redeemed due to the nonlinear growth of the exchange rate. This could lead to unequal percentage gains for users who perform similar calls around the same time. Furthermore, rewrapping the AToken may not be beneficial if it encounters a sudden surge, consequently thwarting the initial objective. The highlighted code snippets show that choosing staticAmount > 0 could result in a lesser amount of AToken being withdrawn when the currentRate is stagnant. In contrast, choosing dynamicAmount > 0 could lead to burning a higher than expected amount of Static Atoken. To mitigate this, the report suggests implementing slippage protection on StaticATokenLM._withdraw so that users could set the minimum AToken amount to receive or the maximum Static Atoken amount to burn.
6. Lack of protection when caling CusdcV3Wrapper._withdraw (Medium) The report discusses potential risks associated with the wComet unwrapping process in the CusdcV3Wrapper contract. It notes that users who invoke CusdcV3Wrapper._withdraw simultaneously could experience different percentage gains due to the non-linear rebasing of comet. Moreover, the report suggests that the rate-determining getUpdatedSupplyIndicies() function is inaccessible to most users who make partial withdrawals, obscuring the ideal unwrapping moment. The document provides a detailed code walkthrough, demonstrating how the value of baseSupplyIndex influences the burn amount, with lower values resulting in higher principalValueSupply and therefore a smaller burn quantity. To remedy these issues, the report proposes implementing slippage protection on CusdcV3Wrapper._withdraw, allowing users to set the minimum comet to receive or the maximum wComet to burn.

Approach taken in evaluating the codebase

Going through the recommended specs and links is of paramount importance prior to going over the codebases line upon line. I started with the generic contracts and then moved on to tackling the specific plugins one after another. It has also helped me smell out some bugs by reading past related audit reports as I pieced together the puzzles. Deep diving into the code logic was fun and satisfying when noticing how the flaws could be so exploited.

Architecture recommendations

The codebase could do better with adequate and complete NatSpec along with some thorough flow charts to help link up essential flows/calls. There have been some redundant and unneeded codes presented in the Gas report. Touching up the codebase with the low and non-critical feedback from the QA report would also help amplify code robustness.

Codebase quality analysis

The Reserve Protocol features one of the most sophisticated and well-structured codebases in the industry. Having prior knowledge of the previous audits will not make it enough to revisit the documentation and specifications. You would assuredly gain a better and often another facet of understanding of the business logic particularly when it relates to Dimensional Analysis.

Centralization risks

There are some centralized risks associated with the vendors but they are deemed out of scope here. Nonetheless, these vendors are mostly reputable third-party contracts that have relatively been battled tested and should not pose much of a concern at this juncture. Perhaps, the protocol should look into implementing some form of emergency contract and/or key function pausing/freezing and be prepared for the worse.

Mechanism review

The Reserve Protocol, with its intricate design and interlocking mechanisms, presents unique challenges for ensuring the stability and security of its network. The protocol has addressed several key concerns raised in previous reports, showcasing its commitment to maintaining a safe and reliable platform for its users.

However, the protocol's complexity necessitates ongoing vigilance. The Token Transfer Mechanism, Vault Control Mechanism, and Withdrawal Mechanisms, while remedied, need constant monitoring to prevent the re-emergence of issues such as inaccurate accounting of rewards or unauthorized asset control.

Moreover, the Token Minting and Price Retrieval Mechanisms require careful handling to avoid potential reentrancy attacks and misvaluation of function calls. Any discrepancies in these systems could lead to an unpredictable token supply and excessive issuance of RTokens, thus destabilizing the entire ecosystem.

Systemic risks

Limited risks will always prevail considering the majority of current designs have not gone live yet. The previous audit on governance currently falling also under the plugins/*, when more elaborately worked upon and implemented, should help defray all anticipated threats. I think the adoption of Revenue Hiding via AppreciatingFiatCollateral.sol is a smart approach overall to minimize the likelihood of having collaterals defaulted to DISABLED. Coupled with good collateral rewards revenue incentives, the high participation of RSR stakers that enhances over-collateralization would assuredly make the system fully established and intact from getting anywhere near to an undesirable haircut.

Time spent

72 hours

Time spent:

72 hours