Aerobloc / Aerobloc-Contracts / Sep2024

Hacken30 Sep 2024Files named in the report

Audit

Hacken reviewed Aerobloc (Aerobloc-Contracts / Sep2024) on 30 Sep 2024: 30 findings, 1 critical; all marked fixed.

1 critical finding, fixed

Open the report Open Aerobloc’s security record

Report hosted on hacken.io.

Trustblock does not review code.

Findings30

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	1
High	0	0	1
Medium	0	0	2
Low	0	0	26

CriticalSandwich Attack in postReward Function Allows A...Fixed

The `postReward()` function in `StakingV2.sol` is used to distribute new rewards proportionally to the amount each staker has staked. The function calculates a new `rewardIndex` based on the total staked amount and the newly distributed reward amount. However, this function is vulnerable to a sandwich attack where an attacker can stake a large number of tokens just before the reward is distributed, receive a disproportionate share of the rewards, and then quickly unstake their tokens. The vulnerability arises because the `postReward()` function does not account for the time that the tokens have been staked, allowing newly staked tokens to be included in the reward calculation immediately. This allows attackers to game the system by temporarily increasing their stake, receiving a significant portion of the rewards, and then withdrawing their stake, effectively gaining free or disproportionate rewards. The `postReward()` function in `StakingV2.sol` is defined as follows: The vulnerability is due to how `rewardIndex` is calculated: The `rewardIndex` calculation is based on `totalStaked`, which includes any recent stakes, even those added just before the reward distribution. This allows any newly staked tokens to receive rewards immediately without considering how long they have been staked. The attacker can then claim their rewards and unstake immediately, profiting from this loophole. Attackers can exploit the function to gain rewards without having staked for a significant period, which undermines the intended reward mechanism. Genuine stakers may receive fewer rewards than they should because the attacker has claimed an unfair share of the rewards pool.

HighMissing Allowance To The StakingV2 Contract Le...Fixed

In order to transfer ERC20 tokens to the `StakingV2.sol` contract via the `postFee()` and `postReward()` functions, the DAO contract is supposed to give the allowance to the `StakingV2.sol`. However, the current implementation of the `postFee()` function lacks the necessary piece of code responsible for that. It can be seen in the code snippets below. As a consequence, if the `StakingV2.sol` contract has some staking balance, which is controlled by the `totalStaked > 0` condition, the function `postFee()` will revert if it will try to transfer ERC20 assets to the `StakingV2.sol` contract.

MediumLack of Time Dependency in calcFeeDiscount Func...Fixed

The `calcFeeDiscount()` function in `Dao.sol` is designed to provide a fee discount based on the amount of `$AERRO` tokens staked by a user. However, the current implementation calculates the discount solely based on the total amount of `$AERRO` staked without considering the staking duration. This discrepancy allows users to exploit the function by temporarily increasing their staked amount through flash loans or by borrowing tokens from any lending protocol where `$AERRO` tokens are available. The function calculates the discount percentage using: The `getTotalStakedOf` function from `StakingV2.sol` simply returns the total staked tokens without any time-based consideration: Since the discount calculation does not account for how long the tokens have been staked, and he `StakingV2.sol` contract does not have any time-lock mechanism, which allows staked tokens to be unstaked in the same transaction. a user can borrow a large amount of `$AERRO` tokens via flash loans or from a lending protocol, stake them to obtain a significant fee discount, and then return the borrowed tokens. This results in reduced fees and potentially higher profits, especially if the cost of the flash loan or borrowing is less than the savings from the discounted fees. Users can manipulate the staking amount temporarily to gain significant fee discounts without the intended long-term commitment, undermining the protocol’s economic model. The protocol could lose revenue from fees as users exploit this vulnerability to gain illegitimate discounts.

MediumProposal Duration Miscalculation Leads to Exten...Fixed

The `PROPOSAL_DURATION` constant in `Dao.sol` is currently used to determine the expiry of proposals based on block numbers. The current implementation assumes a block time of approximately 0.3 seconds, which is unrealistic for most blockchains, including Polygon, Binance Smart Chain (BSC), Avalanche, and Arbitrum. These blockchains have average block times ranging from 1 to 3 seconds. As a result, the proposal expiry duration, which is intended to represent 7 days, ends up being significantly longer than expected due to this incorrect assumption about block times. Affected Code: `deleteProposal()` function: The `PROPOSAL_DURATION` is set as: This calculation is based on an incorrect assumption of a block time of 0.3 seconds. For example, on Polygon, which has an average block time of 2 seconds, the actual expiry period would be around 47 days instead of the intended 7 days. This leads to proposals remaining active much longer than intended, which could affect governance and decision-making processes within the DAO.

LowEvents Not Properly Indexed and Redundant Event...Fixed

In Solidity, indexing event fields enables more efficient querying and filtering of events, which is particularly beneficial when dealing with address fields. Indexing allows off-chain tools like blockchain explorers and indexing services to efficiently filter and parse events, making it easier to track contract activities. However, several events in the contracts (`LandingCharges.sol`, `StakingV2.sol`, `Token.sol`, `Dao.sol`) lack proper indexing of fields, especially address fields. Additionally, the `Token.sol` contract contains redundant events (`MintTokenEvent` and `BurnTokenEvent`) that replicate the functionality of existing events (`Transfer` events) already defined in OpenZeppelin's `ERC20` implementation. Affected Code: `LandingCharges.sol`: `StakingV2.sol`: `Token.sol`: `Dao.sol`: Without indexed fields, off-chain tools cannot efficiently filter and track specific events, which can hinder the ability to monitor and analyze contract activities.

LowFloating PragmaFixed

The contracts in the project use a "floating pragma" statement: `pragma solidity ^0.8.26;`. A floating pragma allows the contracts to be compiled with any compiler version that is `0.8.26` or higher. While this might seem convenient during development, it introduces risks for the deployed contract. Using a floating pragma can lead to compatibility and stability issues because future Solidity versions might introduce breaking changes, deprecate certain features, or introduce new bugs. This could result in the contract being compiled in an unintended way or making it vulnerable to issues not present in the intended compiler version.

LowImplicit Default VisibilityFixed

In Solidity, if a visibility modifier is not explicitly specified for a state variable, it defaults to `internal`. While this might be acceptable in certain cases, it is considered a best practice to always define visibility explicitly. This makes the code more readable and less prone to misunderstandings, ensuring that the developer's intentions are clear. The affected code contains variables with implicit default visibility, which could confuse auditors or other developers about the intended scope of these variables. Explicitly specifying visibility (e.g., `internal`, `private`, `public`) helps improve code clarity and maintainability. Affected Code: `StakingV2.sol`: `Token.sol`: `Dao.sol`: A lack of explicit visibility could lead to unintended use or assumptions about the accessibility of the variables.

LowImproper Reentrancy ProtectionFixed

The `Dao.sol` contract currently uses a custom `_isFeesSplitterLocked` flag in the `postFee()` function for reentrancy protection. However, this flag is incorrectly managed, as it is reset (`_isFeesSplitterLocked = false;`) before all external calls are completed. Additionally, the `actionExternalTransfer()` function lacks any reentrancy protection and, directly calls `executeExternalTransfer()`, which performs external calls without any safeguards. Affected Code: `postFee()` Function: `actionExternalTransfer()` and `executeExternalTransfer()` Functions: The `actionExternalTransfer()` function calls `executeExternalTransfer()` without any reentrancy protection, and `executeExternalTransfer()` performs an external transfer: `executeExternalTransfer()` Function (External Call Part): The `executeExternalTransfer()` function performs an external token transfer without any reentrancy protection.

LowImproper Reentrancy ProtectionFixed

LowInadequate Validation in deleteProposal FunctionFixed

The `deleteProposal()` function is intended to allow an admin to delete a proposal after it has expired. However, it suffers from several validation issues: [object Object],[object Object],[object Object] Affected Code:

Show 10 more

LowInconsistent Error Handling Using Both require(...Fixed

The `Dao.sol` contract uses a mix of `require()` statements with string messages and custom errors for error handling. This inconsistency can lead to confusion and reduced code readability and maintainability. Custom errors are generally preferred in Solidity for gas efficiency and more structured error handling. Using both methods interchangeably creates inconsistency and makes the code harder to understand and maintain. The specific instances where `require()` statements are used with string messages, while the rest of the contract predominantly uses custom errors, are: [object Object] [object Object] Inconsistent error handling can lead to maintenance issues, especially as the codebase grows.

LowInefficient Reuse of keccak256 Hash Calculation...Fixed

In the `Dao.sol` contract, different proposal types are identified by calculating the `keccak256` hash of strings such as "External Payment", "Admins List", and "Splits" directly in various parts of the code. This approach results in the same `keccak256` hash being computed multiple times, leading to inefficiencies in both gas consumption and code readability. For instance, the code snippet shows that the `keccak256` hash is recalculated each time a proposal type is assigned or compared: This repeated calculation of `keccak256` not only leads to additional gas costs but also introduces a higher likelihood of human error if the same string is misspelled in different places.

LowLack of Validation in createNewAdminsScheme Fun...Fixed

The `createNewAdminsScheme()` function in `Dao.sol` allows an admin to propose changing the list of admins in the DAO. However, the function does not validate the `newAdmins` input list to ensure it meets requirements, such as having a minimum number of new admins. Specifically, it allows for creating a proposal with an empty `newAdmins` list: If a proposal with an empty `newAdmins` list is executed, all current admins would be removed, and no new admins would be appointed. This would leave the DAO contract in an unmanageable state where no one would have the authority to create new proposals, approve transactions, or manage the DAO, effectively locking all functionality of the DAO.

LowLack of Validation in createNewFeesSplitScheme ...Fixed

The `createNewFeesSplitScheme()` function in `Dao.sol` allows an admin to create a proposal for updating the `feeSplits` array. However, the function does not validate individual values in the `newSplits` array to ensure they are non-zero. This can lead to scenarios where zero values are included in the new splits, such as `[0, 50, 50]`. When such a proposal is executed through the `executeNewFeesSplitScheme()` function, the old `feeSplits` array is deleted, and only non-zero values from the new splits are retained. This can cause the `feeSplits` array to shrink in size. The `feeSplits` array is initially set to contain exactly three elements during the contract's initialization. In the `postFee()` function, `feeSplits` is used to calculate different amounts, assuming there are always three elements: The function `createNewFeesSplitScheme()` allows proposals for changing the `feeSplits` without ensuring all values are non-zero: The `executeNewFeesSplitScheme()` function then deletes the old `feeSplits` and retains only non-zero values from the new splits, which can shrink the array size: If the `feeSplits` array length becomes less than three, creating a valid proposal to restore the array to three elements would be impossible. This would result in an irrecoverable state where the DAO could no longer adjust the fee splits, permanently locking the DAO's ability to manage fee distribution.

LowLoss of Precision in postFee Function Due to In...Fixed

The `postFee` function in `Dao.sol` splits a given `amount` into three parts: `amountToBurn`, `amountToStakingReward`, and `amountToTreasury` based on predefined percentages (`feeSplits`). However, the current implementation uses integer division to calculate these split amounts, which leads to a loss of precision due to truncation of any fractional remainders. In Solidity, integer division rounds down to the nearest integer, causing potential discrepancies where the sum of `amountToBurn`, `amountToStakingReward`, and `amountToTreasury` does not exactly equal the original `amount`. This can lead to minor discrepancies that accumulate over time. Affected Code: The loss of precision could result in minor differences that accumulate over time, leading to discrepancies in the expected versus actual distribution of fund

LowLoss of Precision in postFee Function Due to In...Fixed

LowMissing disableInitializers() Can Lead to Unwan...Fixed

The `Dao.sol` is an upgradable contract. After an implementation contract is deployed on the blockchain, its implementation must be called by the development team via the proxy in order to set up the basic functionalities of the contract. However, the absence of a [`_disableInitilizers()`](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract) call in the `constructor()` of implementation contract opens the possibility for each implementation to be directly initialised by an external actor.

LowMissing Zero Address CheckFixed

In both `Dao.sol` (`initialize` function) and `LandingCharges.sol` (constructor), there is no check to ensure that admin addresses in the `adminsInit` array are non-zero. This allows `address(0)` to be assigned as an admin, which is invalid and cannot perform administrative actions. Affected Code: `Dao.sol` `LandingCharges.sol`

LowPresence of Debugging Code and Commented Import...Fixed

The `Dao.sol` contract contains debugging code (`console.log` statements) and commented-out imports from the Hardhat development environment, which are not intended for use in a production environment. The `console.sol` library, used with Hardhat for development, allows developers to print output to the console for debugging purposes during local testing. However, these debugging statements should not be present in the deployed contracts as they can bloat the contract size, reduce code readability, and introduce unnecessary complexity. The following sections of code are affected: Commented-out Imports: Debugging `console.log` Statements in `createNewFeesSplitScheme`: Debugging `console.log` Statements in `actionNewFeesSplitScheme`: Debugging `console.log` Statement in `executeNewFeesSplitScheme`: Debugging code and commented-out imports reduce code quality and readability.

LowPublic Functions Not Called Internally Should B...Fixed

Several functions in the `Dao.sol` and `LandingCharges.sol` contracts are declared as `public` but are never called internally within the contract. These functions are intended for external access only. The functions that should be declared `external` instead of `public` are: `Dao.sol`: `LandingCharges.sol`: Declaring functions as `external` makes the intended use clearer to developers, auditors, and users, indicating that the function is meant to be called from outside the contract.

Show 10 more

LowRedundant Else BlocksFixed

The `Dao.sol` contract contains several instances of redundant `else` blocks where the `if` block already includes a `return` or `revert` statement. The use of an `else` block in these cases is unnecessary and adds an additional level of nesting, making the code harder to read and maintain. This issue is found at multiple locations in the contract, such as when returning discount percentages or reverting based on conditions. Instances of Redundant Else-Blocks: [object Object] [object Object] Redundant `else` blocks increase the nesting level and reduce code readability.

LowRedundant Getters Decreases Code Quality And Ga...Fixed

The `Dao.sol` introduces additional getter functions which negatively affect the overall code quality and as a result gas costs. For instance: `getAdmins()`, `getFeeSplits()` and `getProposalsIds()` functions for the publicly visible state variables which already have automatically generated getter functions including `admins`, `feeSplits` and `proposalIds`. The previously mentioned getter functions can be seen in the code snippets below: `getAdmins()` `getFeeSplits()` `getProposalsIds()`

LowRedundant Validation ChecksFixed

In the `Dao.sol` contract, there are redundant validation checks within the internal functions `executeExternalTransfer()`, `executeNewAdminsScheme()`, and `executeNewFeesSplitScheme()`. These functions are only called by `actionExternalTransfer()`, `actionNewAdminsScheme()`, and `actionNewFeesSplitScheme`() respectively, which already perform the necessary validation checks. Affected Code: `createExternalTransfer()` `actionExternalTransfer`, and `executeExternalTransfer()`: [object Object],[object Object] `createNewAdminsScheme()`, `actionNewAdminsScheme()`, and `executeNewAdminsScheme()`: [object Object],[object Object] `createNewFeesSplitScheme()`, `actionNewFeesSplitScheme()`, and `executeNewFeesSplitScheme()`: [object Object],[object Object] Revalidating the same conditions inside the internal functions leads to code duplication, reduced readability, and unnecessary gas costs. These redundant checks should be removed to streamline the contract logic and optimize gas usage.

LowSingle-Step Ownership Transfer Can Lead to Loss...Fixed

The `StakingV2.sol` and `Token.sol` contracts currently use a single-step process for transferring ownership. This approach allows ownership to be transferred to any address, including incorrect or invalid ones (e.g., due to a typo). If ownership is mistakenly transferred to an address that cannot manage it (such as a non-existent address or a contract without an appropriate `onlyOwner` function), it could result in a loss of control over critical functions of the contract. It can be seen in the `transferOwnership()` function of the `Ownable.sol` in the code snippet below: The `Ownable2Step` and `Ownable2StepUpgradeable` contracts from OpenZeppelin introduce a safer, two-step ownership transfer process where the recipient must actively accept ownership. This prevents accidental transfers by ensuring the new owner address is valid and capable of managing the contract.

LowThe Lack of Validation Can Lead To The Reverted...Fixed

In the current implementation of the `Dao` contract, the function `createExternalTransfer()` allows to create pending external transfers. It can be seen in the code snippet below. The pending transfers created by the aforementioned function then can be revoked or approved by the admins via `actionExternalTransfer()` and eventually executed via the `executeExternalTransfer()` However, due to the fact that the `createExternalTransfer()` does not take into account the portion of the treasury, which is already used for the pending transfers, there might be not enough treasury assets to cover all of the transfers.

LowThe totalStaked > 0 Condition Prevents Donation...Fixed

The admin of the system can transfer a portion of the assets to the `StakingV2` contract using `Dao.postFee()` function, which then should call `StakingV2::postReward()` for the transfer. However, the condition in the current version of the `postReward()` function ensures that no transfers can be made without previous stakes to the system. It is demonstrated in the code snippets below. As a consequence, the funds are not sent to the `Staking` contract, but left in the DAO contract if the `totalStaked` variable equals to zero. Moreover, it will not be possible to recover the funds via voting due to the fact that they are not recorded in the `treasury` balance, which only records the `amountTreasury` as it can be seen in the code snippet below.

LowUnnecessary Initialization of Variables to Defa...Fixed

In Solidity, when variables are declared but not explicitly initialized, they automatically receive default values based on their type. For example, `uint` variables default to `0`, `bool` variables default to `false`. In the `Dao.sol` contract, several instances of this unnecessary initialization can be found, leading to inefficient use of resources. Specifically, the variables `_isFeesSplitterLocked`, `externalTransferDetails[uniqueID].confirms`, `externalTransferDetails[uniqueID].executed`, `proposalStatus[uniqueID]`, `scheme.confirms`, and `scheme.executed` are explicitly set to their default values of `false` or `0`, which is redundant. Affected Code: Initialization of `_isFeesSplitterLocked` in `initialize()`: Initialization of `externalTransferDetails` fields and `proposalStatus` in `createExternalTransfer()`: Initialization of `scheme` fields and `proposalStatus` in `createNewAdminsScheme()` and `createNewFeesSplitScheme()`: Explicitly initializing variables to their default values can make the code less clean and more challenging to read, as it introduces unnecessary operations that do not contribute to the functionality.

LowUnsafe Use of transfer()/transferFrom() with IE...Fixed

The `StakingV2.sol` and `Dao.sol` contracts use `IERC20`'s `transfer()` and `transferFrom()` methods directly to handle token transfers. Some tokens, like Tether (USDT) on L1, do not fully comply with the ERC20 standard. Specifically, their `transfer()` and `transferFrom()` functions do not return a boolean value as expected by the standard; instead, they have no return value. When these tokens are cast to `IERC20` and their functions are called, the calls may revert because the function signatures do not match. Affected Code: [object Object] [object Object] Direct use of `transfer()` and `transferFrom()` methods could cause reverts when interacting with non-standard tokens like USDT, leading to transaction failures. The current implementation also includes manual checks for allowance and balance, which is redundant. Example from `StakingV2.sol`: Using OpenZeppelin's `SafeERC20` library not only provides safer and more concise code but also ensures compatibility with both standard-compliant and non-compliant tokens by handling potential edge cases internally.

LowUnused Error DefinitionsFixed

The unused error definitions found in the `Dao.sol` contract are as follows: Both `deploymentGoneWrong` and `indexNotFound` are not referenced elsewhere in the contract, making them redundant. According to Solidity best practices, unused code should be removed to optimize contract size and improve readability.

LowUse of Generic uint Type Instead of Explicitly ...Fixed

In Solidity, it is considered best practice to explicitly define the bit size of integer types such as `uint` and `int` to enhance code readability, clarity, and security. Using `uint` or `int` without specifying a bit size defaults to `uint256` or `int256`, but this is not immediately obvious to developers, especially in a codebase that involves multiple contracts and integer sizes. The lack of explicit declaration can lead to ambiguity, making the code harder to read and maintain, especially during audits. Several instances in `LandingCharges.sol`, `StakingV2.sol`, `Token.sol`, and `Dao.sol` use `uint` without specifying the bit size. This practice can be confusing, as it isn't immediately clear whether the intention is to use `uint256` or a smaller-sized integer. Consistent use of explicit bit sizes, such as `uint256` or `int256`, ensures that the code is clear, reducing the potential for misunderstandings and mistakes. Affected Code: `LandingCharges.sol`: `StakingV2.sol`: `Token.sol`: `Dao.sol`: