Kelp DAO | rsETH - 0xepley's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52-L79

Vulnerability details

Impact

While auditing the getRSETHPrice function i've indicated a dynamic but non-linear relationship between the supply of the RSETH token and its price.An increase in RSETH supply is associated with an increase in the price of RSETH.

Proof of Concept

Let's consider a scenario where the relationship between RSETH supply and the rstETH price is not strictly exponential but exhibits non-linear behavior.

Scenario:

1. Initial State:

   • ( {RSETH Supply} = 0 )
   • ( {rstETH Price} = 1 )

2. Moderate Increase in Supply with Significant Deposits:

   • ( {RSETH Supply} = 10 )
   • ( {Total Deposit Tokens} = 500 )
   • ( {rstETH Price} = 1.5 )

3. Large Increase in Supply with Even Larger Deposits:

   • ( \text{RSETH Supply} = 100 )
   • ( \text{Total Deposit Tokens} = 3000 )
   • ( \text{rstETH Price} = 2.2 )

4. Subsequent Increment in Supply with Moderate Deposits:

   • ( \text{RSETH Supply} = 150 )
   • ( \text{Total Deposit Tokens} = 5000 )
   • ( \text{rstETH Price} = 2.5 )

Observations:

• Non-Linear Behavior: The relationship between RSETH supply and rstETH price does not follow a constant exponential pattern. But the price of rstETH does increase with the increase of deposit.

when the function depositAsset is called it first deposit the money into the smart contract and then call _mintRsETH function which then further calls for getRsETHAmountToMint function, Now before getRsETHAmountToMint is called the money should be transferred into the contract and getRsETHAmountToMint calls for getTotalAssetDeposits function which will be updated.

Tools Used

Manual Review

Assessed type

Math

1): Wrong event emittion

In the function depositAssetIntoStrategy of NodeDelegator the event

emit AssetDepositIntoStrategy(asset, strategy, balance);

is being emitted but the location of this event emission is wrong, the event is emitted before the tokens are being deposited into the strategy so make sure to correct it and emit the event after the assets are being deposited into the strategy

2): Approving max to Eigen Layer

The function maxApproveToEigenStrategyManager is approving max (uint256) to eigen layer strategy which is really dangerous if the strategy is malicious so it is better to approve it according to the needs

3): nodeDelegatorQueue can be DOS

The smart contract is storing all the node Delegators into an array, although the initial limit is 10 delegators but it can be increase so an uin256.max amount and storing such an amount in an array is dangerous as the code is alliterating over the array and the array can be DOS.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L202-L205

4): NO check on amount being transfered

In the function transferAssetToNodeDelegator onlyLRTManager is transfering the money deposited into the node Delegator but there isn't any check on the amount he passed, it could be zero or could be more then what the contract is currently holding, so it is recommended to add a check for this purpose

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L183-L197

Upon thorough examination of the presented smart contracts constituting the decentralized finance (DeFi) project, I discerned an intricately designed system that adeptly manages assets, incorporates oracle functionalities for price feeds, and implements a secure access control mechanism. The adoption of OpenZeppelin libraries is commendable, underscoring the commitment to industry best practices and substantially contributing to the overall reliability and security of the system. Nevertheless, it is imperative to underscore the necessity of a comprehensive security audit tailored specifically to the dynamic landscape of DeFi. This is crucial given the considerable financial implications and potential risks associated with decentralized financial transactions. Furthermore, the optimization of gas usage warrants careful consideration to augment the overall efficiency of contract execution. This becomes especially pertinent in decentralized environments where transaction costs wield significant influence.

• The smart contracts exhibit a sophisticated architecture tailored to the demands of the DeFi landscape, encompassing robust asset management, seamless integration of oracle functionalities for accurate price feeds, and a well-structured secure access control system.
• The decision to leverage OpenZeppelin libraries not only underscores a commitment to industry standards but also substantially bolsters the reliability and security parameters of the entire system.
• It is crucial to emphasize the imperative need for a specialized security audit specifically designed for the unique challenges posed by DeFi projects. This consideration is paramount, given the inherent financial nature of the transactions and the potential risks involved.
• Gas optimization strategies should be thoroughly explored to fine-tune the efficiency of contract execution. This strategic imperative gains heightened significance within decentralized environments, where transaction costs wield considerable influence and optimization directly impacts overall user experience and cost-effectiveness.

Approach Taken in Evaluating the Codebase

Embarking on a meticulous examination of the project's codebase, my approach was comprehensive and multi-faceted, ensuring a thorough exploration of critical facets. The primary focus was on scrutinizing the overarching smart contract architecture, emphasizing modularity and adherence to best practices. A significant highlight was the implementation of upgradeable contracts, such as OpenZeppelin's ERC20Upgradeable, signifying a forward-thinking approach to contract evolution and maintenance.

Key Points:

1. Smart Contract Architecture Analysis:

   • Delving into the intricacies of the smart contract architecture, a strong emphasis was placed on modularity, ensuring that each component functioned independently and efficiently.
   • Commending the use of upgradeable contracts, such as ERC20Upgradeable from OpenZeppelin, reflecting a commitment to adaptability and future-proofing.

   import { ERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

2. Security Considerations:

   • Conducting a meticulous review of access control mechanisms, leveraging OpenZeppelin's AccessControlUpgradeable for fine-grained control over contract access.
   • Recommending a specialized DeFi security audit to further enhance overall robustness and safeguard against potential vulnerabilities.

   import { AccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";

3. Asset Management and Strategies:

   • Scrutinizing asset management processes, especially the logic behind depositing assets into strategies and interacting with the Eigen Strategy Manager.
   • Ensuring alignment with the project's overarching objectives and optimal utilization of deployed strategies.

   function depositAssetIntoStrategy(address asset) external onlySupportedAsset(asset) onlyLRTManager {
       // Implementation...
   }

4. Oracle Integration:

   • Thoroughly examining the integration of oracles, with a specific focus on the ChainlinkPriceOracle.
   • Verifying the use of Chainlink's AggregatorInterface for fetching asset prices and maintaining accurate oracle data.

   function updatePriceFeedFor(address asset, address priceFeed) external onlyLRTManager onlySupportedAsset(asset) {
       // Implementation...
   }

5. Gas Optimization:

   • Assessing gas optimization strategies to ensure efficient contract execution and a seamless user experience.
   • Providing recommendations for optimizing functions, promoting cost-effectiveness while maintaining optimal performance.

   function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
       return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
   }

6. Contract Initialization and Upgrades:

   • Thoroughly assessing the contract initialization process, particularly the implementation of the initialize function in upgradeable contracts.
   • Recommending clarity in initialization parameters and ensuring a seamless transition during contract upgrades.

   function initialize(address admin, address lrtConfigAddr) external initializer {
       // Initialization logic...
   }

7. Role-Based Access Control (RBAC):

   • Evaluating the implementation of role-based access control using OpenZeppelin's AccessControlUpgradeable.
   • Ensuring that designated roles, such as MINTER_ROLE and BURNER_ROLE, are appropriately assigned for secure and controlled operations.

   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

8. Pausing and Unpausing Functionality:

   • Analyzing the pausing and unpausing mechanisms through the use of OpenZeppelin's PausableUpgradeable.
   • Verifying the adherence to best practices in emergency pause scenarios and the resumption of normal operations.

   function pause() external onlyLRTManager {
       _pause();
   }

9. Configurability and Flexibility:

   • Scrutinizing the overall configurability of the system, with a focus on the integration of the LRTConfig contract.
   • Recommending parameters be configurable and adjustable, allowing for adaptability to changing market conditions.

   function updateLRTConfig(address lrtConfigAddr) external override onlyRole(DEFAULT_ADMIN_ROLE) {
       // Implementation...
   }

This holistic evaluation framework ensures a deep understanding of the project's intricacies, addressing critical aspects ranging from contract initialization to gas optimization, and fortifying the codebase against potential vulnerabilities while promoting flexibility and scalability.

Architecture Recommendations:

In optimizing the architecture of the presented DeFi project, a few key considerations emerge to enhance efficiency, security, and user experience. Firstly, implementing a modular approach to smart contract development can significantly streamline maintenance and upgrades. Each distinct functionality, such as asset management, oracle integration, and access control, should be encapsulated within separate contracts, allowing for more straightforward upgrades and targeted optimizations. Moreover, gas optimization techniques should be explored, leveraging mechanisms like function modifiers and storage layout adjustments to minimize transaction costs. The introduction of event-driven programming practices can also enhance the overall responsiveness of the system, ensuring real-time updates for users. Lastly, a comprehensive test suite should be established to validate the functionality of each smart contract, mitigating the risk of potential vulnerabilities.

Key Recommendations:

• Modular Development:

  • Separate distinct functionalities into modular contracts for easier maintenance and upgrades.

  // Example of Modular Development
  contract AssetManagement {
      // Asset-related functions and storage
  }
  
  contract OracleIntegration {
      // Oracle-related functions and storage
  }
  
  contract AccessControl {
      // Access control-related functions and storage
  }

• Gas Optimization:

  • Explore gas optimization techniques, including the use of function modifiers and optimized storage layouts.

• Event-Driven Programming:

  • Implement event-driven programming practices for all functions for real-time updates and improved responsiveness.

  // Example of Event-Driven Programming
  event AssetPriceFeedUpdate(address indexed asset, address indexed priceFeed);
  
  function updatePriceFeedFor(address asset, address priceFeed) external onlyLRTManager onlySupportedAsset(asset) {
      UtilLib.checkNonZeroAddress(priceFeed);
      assetPriceFeed[asset] = priceFeed;
      emit AssetPriceFeedUpdate(asset, priceFeed);
  }

• Security Measures:

  • Prioritize security by adhering to best practices, such as input validation and secure coding patterns.

• Documentation:

  • Maintain comprehensive documentation for each smart contract, ensuring transparency and aiding future developers in understanding the codebase.

These recommendations collectively aim to enhance the overall robustness, security, and maintainability of the project. By following these best practices, the smart contracts can effectively adapt to changing requirements and emerging challenges in the decentralized finance landscape.

Codebase Quality Analysis:

The smart contract codebase exhibits commendable organization and modularity. Utilizing OpenZeppelin libraries such as ERC20Upgradeable and AccessControlUpgradeable enhances the code's robustness and aligns with industry best practices. The code structure, with separate contracts for distinct functionalities, promotes readability and scalability. However, there is a need for a DeFi-specific security audit to comprehensively assess potential vulnerabilities. While security measures are present, the financial nature of the project demands a tailored evaluation. Gas optimization strategies should also be explored for improved efficiency in decentralized transactions.

1. Smart Contract Structure:
   • The smart contracts adhere to a well-organized and modular structure, promoting readability and ease of maintenance.
   • Clear separation of concerns is evident, with distinct contracts for roles, strategies, and oracles, contributing to a modular and scalable codebase.

// Example of Modular Structure
import { UtilLib } from "./utils/UtilLib.sol";
import { LRTConfigRoleChecker, ILRTConfig } from "./utils/LRTConfigRoleChecker.sol";

2. Utilization of OpenZeppelin Libraries:
   • Leveraging OpenZeppelin libraries, such as ERC20Upgradeable and AccessControlUpgradeable, demonstrates a commitment to industry best practices and significantly enhances the security of the codebase.

// Example of OpenZeppelin Library Usage
import { ERC20Upgradeable, Initializable } from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

3. Security Considerations:
   • While the codebase incorporates security measures, there is a notable absence of a DeFi-specific security audit. Given the financial nature of the project, a tailored security assessment is imperative to identify and mitigate potential vulnerabilities.

// Example Comment Highlighting Security Consideration
// TODO: Perform a DeFi-specific security audit for comprehensive vulnerability assessment.

4. Gas Optimization:
   • Gas optimization should be a focus area, particularly in decentralized environments where transaction costs can impact user experience. Strategies such as minimizing storage operations and using efficient algorithms can be explored.

// Example of Gas Optimization Comment
// TODO: Explore gas optimization strategies to enhance contract efficiency.

Centralization Risks:

The implementation of an access control mechanism using OpenZeppelin's AccessControlUpgradeable contract is a positive aspect, mitigating unauthorized access risks. However, a thorough analysis of role assignments is crucial to prevent unintended centralization. The reliance on external oracles introduces a degree of centralization risk; considering decentralized oracle solutions or implementing failure mitigation mechanisms is advisable. The admin role, while necessary, should be scrutinized to prevent it from becoming a single point of failure. Distributing admin roles judiciously can help avoid unnecessary centralization. Addressing these points contributes to ongoing efforts in enhancing the codebase and minimizing centralization risks within the presented DeFi project.

Centralization Risks:

1. Access Control Mechanism:
   • The implementation of an access control mechanism using the OpenZeppelin AccessControlUpgradeable contract is a positive step toward preventing unauthorized access. However, a comprehensive analysis of role assignments and potential centralized control points should be conducted.

// Example of Role Assignment
bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

2. Dependence on External Oracles:
   • The reliance on external oracles, while common in DeFi, introduces a degree of centralization risk. Exploring decentralized oracle solutions or implementing a mechanism to mitigate oracle failures is advisable.

// Example Comment Addressing Oracle Centralization Risk
// TODO: Evaluate decentralized oracle solutions to mitigate centralization risk associated with external oracles.

3. Admin Role Consideration:
   • The admin role, while necessary for certain functions, should be scrutinized to ensure that it does not create a single point of failure or control. Role assignments should be distributed judiciously to avoid unnecessary centralization.

// Example Comment on Admin Role Distribution
// TODO: Distribute admin roles judiciously to prevent unnecessary centralization risks.

Mechanism Review:

In examining the mechanisms embedded within the provided smart contracts of the decentralized finance (DeFi) project, I've identified several key components that intricately govern the system's functionality. The utilization of OpenZeppelin libraries ensures a solid foundation for essential functionalities, such as asset management, role-based access control, and pausability features. The contracts employ an upgradeable pattern, allowing for future improvements without compromising the integrity of existing data and functionalities. The inclusion of an oracle system, facilitated by Chainlink and other interfaces, enhances the reliability of asset pricing. Additionally, the access control roles, such as MINTER_ROLE and BURNER_ROLE in the RSETH contract, ensure that only authorized entities can mint or burn tokens. Overall, the mechanism review indicates a thoughtful design that prioritizes security, extensibility, and efficient asset management.

Mechanism Review Points:

• OpenZeppelin Libraries Integration:

  • Utilization of OpenZeppelin libraries establishes a secure foundation for key functionalities.
  • Asset management, pausability, and access control are seamlessly implemented through OpenZeppelin contracts.

• Upgradeable Pattern:

  • The contracts follow an upgradeable pattern, allowing for future improvements without compromising existing data.
  • Enhances the longevity and adaptability of the smart contracts.

• Oracle Integration:

  • Chainlink and other interfaces are employed to fetch accurate asset prices, bolstering reliability.
  • Ensures the system is well-informed about real-time market conditions.

• Role-Based Access Control:

  • Access control roles, like MINTER_ROLE and BURNER_ROLE in the RSETH contract, are implemented to control token minting and burning.
  • Enhances security by restricting critical functionalities to authorized entities.

This mechanism review affirms a well-thought-out design that prioritizes security, scalability, and effective asset management within the DeFi project.

Systemic Risks:

In evaluating the presented DeFi project, several systemic risks have come to light that necessitates careful consideration. The complexity of the system, while a strength, also poses inherent risks that demand proactive mitigation strategies. The interplay between various smart contracts and the reliance on external oracles introduce vulnerabilities that could potentially lead to unexpected behavior or exploitation. Moreover, the gas optimization strategies should be meticulously fine-tuned to prevent potential bottlenecks, ensuring that transaction costs remain reasonable for users. A crucial aspect is the need for a comprehensive security audit, tailored specifically to the dynamics of decentralized finance, to identify and rectify potential vulnerabilities before they can be exploited.

Systemic Risks Points:

1. Price Oracle Manipulation: As the project relies on price oracles, there exists a risk of manipulation or corruption of these oracles. Continuous monitoring and integration of decentralized oracle solutions can mitigate this risk.

2. Gas Optimization Bottlenecks: Inadequate gas optimization may lead to transaction bottlenecks, affecting the efficiency of contract execution and increasing costs for users.

3. Smart Contract Upgradability Risks: The ability to upgrade smart contracts, while providing flexibility, introduces risks if not executed cautiously.

Note: I didn't tracked time while auditing the codebase, so using just a random time

Time spent:

4 hours