AppTreasury Contract - Rezerve Money

The AppTreasury contract inherits from multiple modules like AppAccessControlled, IAppTreasury, PausableUpgradeable and ReentrancyGuardUpgradeable but only calls the initializers for AppAccessControlled (__AppAccessControlled_init()) and PausableUpgradeable (__Pausable_init()) within the initialize() function. This leaves ReentrancyGuardUpgradeable uninitialized.

In the OpenZeppelin upgradeable contract pattern, initializers are used to set up state variables and other configurations that would typically be done in a constructor for non-upgradeable contracts. Failing to explicitly call the __ReentrancyGuard_init() initializer means that any future changes to the ReentrancyGuardUpgradeable contract's initialization logic will not be applied to this contract.

It is recommended to initialize all inherited upgradable contracts. More specifically, explicitly call the __ReentrancyGuard_init() function in the initialize() function to ensure all inherited modules are properly initialized. This approach aligns with standard practices for using the OpenZeppelin upgradeable pattern and helps future-proof the contract against potential changes in the underlying library.

SOLVED: The Reserve Money team fixed this finding in commit cb40ced by explicitly calling the __ReentrancyGuard_init() function in the initialize() function as recommended.

The AppTreasury contract assumes that calling safeTransferFrom() always results in exactly _amount tokens arriving, but fee-on-transfer tokens deduct a fee on each transfer, so the actual amount received can be less than _amoun. Consequently, tokenValueE18(_token, _amount) could overestimate the treasury's reserves and causes over-minting of the protocol's token, leading to inaccurate accounting and potential inflation.

The contract assumes that the amount specified in transfer operations equals the amount actually received. This assumption is visible in the deposit() function:

function deposit(uint256 _amount, address _token, uint256 _profit)
  external
  override
  nonReentrant
  whenNotPaused
  onlyReserveDepositor
  returns (uint256 send_)
{
  require(enabledTokens[_token], "Treasury: invalid token");

  IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);

  uint256 value = tokenValueE18(_token, _amount);

  // mint app needed and store amount of rewards for distribution
  send_ = value - _profit;
  app.mint(msg.sender, send_);

  _totalReserves += value;
  ...

• Assumed vs. Actual Received: The code uses the requested _amount to compute value, not the actual balance change, so fees charged by the token contract are ignored.

• Over-Minting Risk: Because the treasury mints based on the higher, assumed value, depositors of fee-on-transfer tokens receive more protocol tokens than warranted, inflating supply.

• Reserve Miscalculation: The _totalReserves counter increments by the assumed value, causing reserve tallies to exceed on-chain balances and obscuring true collateral levels.

It is recommended to either add support to fee-on-transfer tokens by verifying the actual transferred amount or explicitly document in the contract that fee-on-transfer tokens are not supported.

RISK ACCEPTED: The Reserve Money team accepted the risk of this finding indicating that they won't use fee on transfer tokens.

During the security assessment, it was identified that some functions in the smart contract lack proper input validation, allowing critical parameters to be set to undesired or unrealistic values. This can lead to potential vulnerabilities, unexpected behavior, or erroneous states within the contract. Examples include:

• The functions deposit(), withdraw() and manage() are not checking if the _amount argument is 0.

• The _profit argument in deposit() is not validated against the calculated value. Therefore, it could cause an immediate, low-level panic revert without a descriptive error message.

• The initialize() function is not verifying if the _authority argument is address(0).

This list is not exhaustive. It is recommended to conduct a comprehensive review of the codebase to identify and assess other functions that may require additional input validation. Ensuring appropriate checks are in place for critical parameters will enhance the overall reliability, security, and predictability of the contracts.

To mitigate these issues, implement input validation in all constructor functions and other critical functions to ensure that inputs meet expected criteria. This can prevent unexpected behaviors and potential vulnerabilities.

RISK ACCEPTED: The Reserve Money team accepted the risk of this finding indicating that they won't use fee on transfer tokens.

The AppTreasury contract follows an upgradeable pattern, indirectly inheriting from the Initializable module from OpenZeppelin. In order to prevent leaving the contracts uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers() function in the constructor to automatically lock the contracts when they are deployed. However, the upgradeable contract in scope is missing this function call.

This omission can lead to potential security vulnerabilities, as an uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

Consider adding a constructor and calling the _disableinitializers() method within the contracts to prevent the implementation from being initialized.

constructor() {
  _disableInitializers();
}

RISK ACCEPTED: The Reserve Money team accepted the risk of this finding indicating that they won't use fee on transfer tokens.

The initialize() function in AppTreasury is using the modifier reinitializer(5), implying this is the fifth upgrade. However, this contract has no prior deployments. In OpenZeppelin’s model, initializer corresponds to version 1, and reinitializer(N) is intended only for genuine Nth-version upgrades. Using reinitializer(5) on a fresh deployment has no effect beyond misleading readers and risking incorrect version assumptions

Furthermore, as per the Initializable convention, parent initialization routines called by child initializers must use the onlyInitializing modifier, ensuring they run only during an initialization context and cannot be invoked arbitrarily. Currently, __AppAccessControlled_init(address) lacks this modifier, allowing it to execute outside the intended initialization phase and potentially misconfigure access control if ever called improperly. For more information, see: https://docs.openzeppelin.com/contracts/5.x/api/proxy#Initializable-onlyInitializing-- and https://www.rareskills.io/post/initializable-solidity

• Use the initializer modifier for the initial setup instead of reinitializer:

function initialize(address _dre, address _dreOracle, address _authority) external initializer {

Reserve reinitializer(N) for each subsequent true upgrade step (e.g., reinitializer(2), reinitializer(3), etc.)

• Guard Sub‐Initializers with onlyInitializing:

Update the __AppAccessControlled_init() function to:

function __AppAccessControlled_init(address _authority) internal onlyInitializing {

ensuring it can only be invoked within an active initializer or reinitializer context.

• Document Initialization Versioning:

Clearly outline in code comments and project documentation the expected initialization/versioning sequence, mapping each function to its intended version, to prevent confusion and enforce correct upgrade practices.

RISK ACCEPTED: The Reserve Money team accepted the risk of this finding.

The enable() function iterates through the entire tokens array to detect duplicates but, upon finding one, still updates the enabledTokens mapping and emits TokenEnabled, resulting in misleading events and redundant on-chain writes.

function enable(address _address) external onlyGovernor {
  require(_address != address(0), "Zero address");

  // RZR should not be enabled as a reserve; as this creates a circular dependency
  require(_address != address(app), "RZR address");

  // add token into tokens array if not already added
  bool isAdded = false;
  for (uint256 i = 0; i < tokens.length; i++) {
    if (tokens[i] == _address) {
      isAdded = true;
      break; //@audit it could just revert here indicating the token is already enabled
    }
  }
  if (!isAdded) tokens.push(_address);

  enabledTokens[_address] = true;
  ...

Furthermore, calculateActualReserves() loops over every entry in tokens, checking enabledTokens each time, even though all tokens in the tokens list should be enabled.

function calculateActualReserves() public view override returns (uint256 reserves) {
  for (uint256 i = 0; i < tokens.length; i++) {
    if (enabledTokens[tokens[i]]) {
      ...

• In enable(), immediately revert on duplicates (e.g. revert TokenAlreadyEnabled()) before setting the mapping or emitting an event:

function enable(address _address) external onlyGovernor {
  require(_address != address(0), "Zero address");
  // RZR should not be enabled as a reserve; as this creates a circular dependency
  require(_address != address(app), "RZR address");

  // add token into tokens array if not already added
  uint256 tokensLength = tokens.length;
  for (uint256 i = 0; i < tokensLength; ++i) {
    if (tokens[i] == _address) revert TokenAlreadyEnabled();
  }
  tokens.push(_address);
  enabledTokens[_address] = true;

• In calculateActualReserves(), assume the tokens array contains only active entries (once duplicates are barred) or filter out disabled tokens via swap-and-pop in disable(), so the loop need not re-check every address.

function calculateActualReserves() public view override returns (uint256 reserves) {
  uint256 tokensLength = tokens.length;
  for (uint256 i = 0; i < tokensLength; ++i) {
      ...

ACKNOWLEDGED: The Reserve Money team accepted the risk of this finding indicating that they won't use fee on transfer tokens.

The contract in scope currently uses a pragma version ^0.8.15, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.15 and less than 0.9.0. It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version of all files to the same version used during development and testing.

SOLVED: The Reserve Money team fixed this finding in commit cd73e8e by locking the Solidity version in use as recommended.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below:

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state:

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in the official Solidity documentation.

ACKNOWLEDGED: The Reserve Money team acknowledged this finding.

The project codebase contains several stylistic inconsistencies and deviations from Solidity best practices, which, while not directly impacting functionality, reduce code readability, maintainability, and adherence to standard conventions. Addressing these inconsistencies can enhance the clarity and professionalism of the code.

• Use of Whole File Imports Instead of Named Imports: Full file imports are used, which may include unnecessary code and reduce clarity.

• Use of public Where external Could Be Used: functions like initialize(), backingRatioE18() or calculateReserves() are declared as public even though it could be declared as external to potentially save gas and adhere to best practices.

Consider implementing the style improvements highlighted above to enhance the readability and consistency of the project.

ACKNOWLEDGED: The Reserve Money team acknowledged this finding.

The project accepts using a Solidity compiler version greater than 0.8.17, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

For example, instead of declaring:

mapping(address => bool) public enabledTokens;

It could be declared as:

mapping(address token => bool enabled) public enabledTokens;

SOLVED: The Reserve Money team fixed this finding in commit 4c4d2c9 by removing the mappings of the contract.

The AppTreasury contract contains an unsafe ERC20 operation that does not use the OpenZeppelin's SafeERC20 library. This operation may fail silently with non-compliant tokens that return false instead of reverting on failure, or tokens like USDT that require resetting allowances to zero before updating them.

function withdraw(uint256 _amount, address _token)
  external
  override
  nonReentrant
  whenNotPaused
  onlyReserveManager
{
  require(enabledTokens[_token], "Treasury: not accepted");

  uint256 value = tokenValueE18(_token, _amount);
  app.transferFrom(msg.sender, address(this), value);
  ...

Note: this instance was only reported as informational because the only unsafe ERC20 function detected was used to interact with the app (RZR) token.

Consider using the OpenZeppelin's SafeERC20 library consistently throughout the codebase.

SOLVED: The Reserve Money team fixed this finding in commit cb40ced by using the OpenZeppelin's SafeERC20 library consistently as recommended.

When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this results in an extra sload operation (100 additional gas for each iteration except the first). For memory arrays, this results in an extra mload operation (3 additional gas for each iteration except the first).

Detecting loops that use the length member of a storage array in their loop condition without modifying it can reveal opportunities for optimization. See the following instances:

function enable(address _address) external onlyGovernor {
  require(_address != address(0), "Zero address");

  // RZR should not be enabled as a reserve; as this creates a circular dependency
  require(_address != address(app), "RZR address");

  // add token into tokens array if not already added
  bool isAdded = false;
  for (uint256 i = 0; i < tokens.length; i++) {
    ...

And:

function calculateActualReserves() public view override returns (uint256 reserves) {
  for (uint256 i = 0; i < tokens.length; i++) {
    ...

Cache the length of the (storage and memory) arrays in a local variable outside the loop to optimize gas usage. This reduces the number of read operations required during each iteration of the loop. See the example fix below:

function enable(address _address) external onlyGovernor {
  require(_address != address(0), "Zero address");

  // RZR should not be enabled as a reserve; as this creates a circular dependency
  require(_address != address(app), "RZR address");

  // add token into tokens array if not already added
  bool isAdded = false;
  uint256 tokensLength = tokens.length;
  for (uint256 i = 0; i < tokens.length; i++) {
    ...

And:

function calculateActualReserves() public view override returns (uint256 reserves) {
  uint256 tokensLength = tokens.length;
  for (uint256 i = 0; i < tokensLength; i++) {
    ...

ACKNOWLEDGED: The Reserve Money team acknowledged this finding.

Both deposit() and withdraw() guard against unapproved tokens but use different revert strings for the same condition:

function deposit(uint256 _amount, address _token, uint256 _profit)
  ...
  require(enabledTokens[_token], "Treasury: invalid token");

And:

function withdraw(uint256 _amount, address _token)
  ...
  require(enabledTokens[_token], "Treasury: not accepted");

This inconsistency can confuse integrators and complicate error handling in UIs and scripts, as they must account for multiple messages for the same failure case.

Unify the revert reason (or migrate to a single custom error) so that both functions use an identical message. This ensures consistent on-chain semantics and simplifies off-chain error handling.

ACKNOWLEDGED: The Reserve Money team acknowledged this finding.