Prepared by:
HALBORN
Last Updated 07/26/2024
Date of Engagement by: June 24th, 2024 - July 3rd, 2024
100% of all REPORTED Findings have been addressed
All findings
10
Critical
2
High
0
Medium
1
Low
2
Informational
5
Bonzo engaged Halborn to conduct a security assessment on their smart contracts beginning on June 24th and ending on July 3rd. The security assessment was scoped to the smart contracts provided in the GitHub repository Bonzo Staking Module, commit hashes and further details can be found in the Scope section of this report.
The protocol utilizes Aave's Safety Module for staking and rewards distribution in WHBAR on the Hedera network. Stakers of any permitted asset by the emission manager can stake their assets in the Safety Module. The protocol ensures that stakers earn rewards in WHBAR, Hedera's wrapped HBAR token. The Safety Module acts as a backstop in case of a shortfall event, enhancing the protocol's security. This system is designed to incentivize users to contribute to the protocol's safety while earning rewards for their participation.
The team at Halborn was provided one week for the engagement and assigned one full-time security engineer to check the security of the smart contract. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.
The purpose of this assessment is to:
Ensure that smart contract functions operate as intended.
Identify potential security issues with the smart contracts.
In summary, Halborn identified some security risks that were mostly addressed by the Bonzo team.
Halborn performed a combination of manual review of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the smart contract assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices.
The following phases and associated tools were used throughout the term of the assessment:
Research into the architecture, purpose, and use of the platform.
Smart contract manual code review and walkthrough to identify any logic issues.
A thorough assessment of safety and usage of critical Solidity variables and functions in scope that could lead to arithmetic related vulnerabilities.
Manual testing by custom scripts.
Graphing out functionality and contract logic/connectivity/functions (solgraph).
Static Analysis of security for scoped contract, and imported functions. (Slither).
Local or public testnet deployment (Brownie, Remix IDE).
| EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
| Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
| Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
| IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
| Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
| Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
| SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
|---|---|---|
| Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
| Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
| Severity | Score Value Range |
|---|---|
| Critical | 9 - 10 |
| High | 7 - 8.9 |
| Medium | 4.5 - 6.9 |
| Low | 2 - 4.4 |
| Informational | 0 - 1.9 |
Critical
2
High
0
Medium
1
Low
2
Informational
5
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| Miscalculated Period Disrupt Emission Schedule Execution | Critical | Solved - 07/12/2024 |
| Wrong Reward Distribution Leads to Double Spend | Critical | Solved - 07/12/2024 |
| Invalid emissionSchedule length check | Medium | Solved - 07/15/2024 |
| Missing duplicate caller addresses check | Low | Solved - 07/12/2024 |
| Hardcoded Testnet EVM Address Instead of Mainnet | Low | Solved - 07/12/2024 |
| Increase PERIOD_DURATION to 30 Days | Informational | Solved - 07/12/2024 |
| Inconsistent Struct Parameter Update | Informational | Solved - 07/12/2024 |
| Usage of Outdated SafeCast | Informational | Acknowledged |
| Use of memory Instead of calldata in Function Variable | Informational | Acknowledged |
| Redundant Parameters in Function _getAssetIndex | Informational | Solved - 07/12/2024 |
// Critical
The current implementation of the _getAssetIndex function in the AaveDistributionManager contract incorrectly calculates the currentPeriod using periodStartTimestamp.sub(lastUpdateTimestamp). Initially, during the first iteration, both periodStartTimestamp and lastUpdateTimestamp are the same, making the difference zero, which works correctly. However, this approach fails when lastUpdateTimestamp is higher or equal to the timestamp of period 1 (index 0). This leads to incorrect period calculations and potential out-of-bounds errors when accessing the emission schedule.
The issue arises because the code does not correctly handle cases where the lastUpdateTimestamp is already in a later period, as shown in the provided diagrams. This flaw was not evident during initial setup testing when lastUpdateTimestamp was set to the current timestamp, corresponding to index 0.
To resolve this critical issue, the currentPeriod should be calculated using the formula DISTRIBUTION_DURATION - (DISTRIBUTION_END - periodStartTimestamp) / PERIOD_DURATION or by maintaining a mechanism to keep track of the last distributed period and calculating the elapsed time difference with the current timestamp.
The new formula needs to be thoroughly tested to ensure it handles all edge cases correctly. This testing should include scenarios where updates are made in periods beyond the initial setup and various edge cases such as period transitions and partial periods.
SOLVED: The Bonzo team solved this issue as recommended.
// Critical
The AaveIncentivesController contract has a vulnerability that results in double spending of rewards. The issue occurs in the claimRewards function, where the contract transfers rewards both as REWARD_TOKEN and as IWHBAR. Specifically, the contract transfers the reward amount to the recipient twice, once via REWARD_TOKEN.transferFrom(REWARDS_VAULT, to, amountToClaim) and again via IWHBAR(_whbarContract).withdraw(REWARDS_VAULT, to, amountToClaim).
This leads to the recipient receiving the reward amount twice, causing financial loss to the rewards vault and effectively doubling the rewards for the claimant.
To fix this issue, the rewards should first be transferred to the contract's address and then transferred to the recipient. Additionally, proper approvals should be set up for the IWHBAR contract to transfer the reward amounts.
By implementing this change, the rewards will be correctly transferred to the recipient only once, and the risk of double spending will be mitigated. Additionally, ensure that the REWARD_TOKEN has been approved for spending by the IWHBAR contract to facilitate the withdrawal. This approach prevents financial loss and maintains the integrity of the rewards' distribution.
SOLVED: The Bonzo team solved this issue as recommended. The REWARDS_VAULT will require to approve AaveIncentivesController to spend the _whbarContract tokens.
// Medium
The AaveDistributionManager contract allows the direct call to the configureAssets function without validating the length of the emissionSchedule. This can lead to an out-of-bounds error when fetching the asset index if an invalid emissionSchedule length is provided. The initialization functions in StakedToken and AaveIncentivesController correctly perform this check, but it should also be enforced in the configureAssets function of AaveDistributionManager to prevent potential vulnerabilities from improper usage.
Add the following check in the configureAssets function of the AaveDistributionManager contract to ensure the validity of the emissionSchedule length:
require(
emissionSchedule.length * PERIOD_DURATION == DISTRIBUTION_DURATION,
'Invalid emission schedule length'
);This will ensure that the emission schedule length is always valid, thereby preventing out-of-bounds errors when accessing the asset index.
SOLVED: The Bonzo team solved this issue as recommended.
// Low
The AaveIncentivesController contract lacks a check in the addCallerAssets function to prevent the addition of duplicate caller addresses. The current implementation uses a list to store whitelisted callers for the onlyCallerAsset modifier, which is not optimized for checking or preventing duplicates. This can lead to inefficiencies and potential issues with duplicate entries. The use of EnumerableSet from OpenZeppelin or a two-struct approach (a mapping for O(1) access and a list for enumeration) would optimize the code and prevent duplicates effectively.
Optimize the addCallerAssets function by using EnumerableSet from OpenZeppelin or a two-struct approach to handle the whitelist of caller addresses. This will prevent duplicate entries and improve the efficiency of the whitelist check.
SOLVED: The Bonzo team solved this issue as recommended. Moreover, a new struct was added to track the assets, this struct makes the addition, removal and most important, the check easier and more efficient for large amount of registered assets.
// Low
The contract contains a hardcoded Ethereum address pointing to a testnet environment instead of the mainnet. This can lead to malfunction or security risks when deployed on the mainnet, as the address will not point to the intended contract or service. As shown in the SaucerSwap contract deployment page, the EVM addresses for WHBAR are:
It is crucial to update the address to the mainnet equivalent before deployment, or use configurable parameters to set the correct environment-specific address dynamically, ensuring proper functionality and security across different environments.
SOLVED: The Bonzo team solved this issue as recommended.
// Informational
The contract contains a TODO comment indicating the intention to increase the immutable PERIOD_DURATION variable to 30 days at a later time. Leaving such TODOs unresolved can lead to inconsistencies and potential issues in the contract's functionality.
It is crucial to address this change promptly by updating PERIOD_DURATION to 30 days or removing the TODO if the change is no longer necessary. Ensuring that all TODOs are resolved enhances the contract's reliability and clarity.
SOLVED: The Bonzo team solved this issue as recommended.
// Informational
The parameter emissionPerSecond of the AssetData struct in AaveDistributionManager has been modified to include an array of different emissions depending on the period, but this update was not reflected in the out-of-scope IStakedToken.sol interface, which is utilized in StakeUIHelper.sol. This inconsistency can lead to integration issues and potential bugs, as the outdated interface does not match the current contract implementation.
It is recommended to update the IStakedToken.sol interface to reflect the changes in the emissionPerSecond parameter, and, if willing to be used, refactor StakeUIHelper.sol accordingly in order to ensure seamless interaction between the contracts and preventing any discrepancies.
SOLVED: The Bonzo team solved this issue as recommended. The IStakedToken was removed.
// Informational
The contract uses an outdated version of SafeCast that allows implicit casting from uint to int. This implicit casting can introduce vulnerabilities and unintended behavior due to the differences in value ranges between unsigned and signed integers. Updating to a newer version of SafeCast that requires explicit casting will ensure safer and more predictable type conversions, mitigating potential risks, and will allow the use of newer and safer compiler versions.
By using explicit type conversions, the contract ensures clarity and prevents unintended behavior, aligning with the safety improvements introduced in recent Solidity versions.
ACKNOWLEDGED: Ignoring as it causes compatibility issues with the older versions of the SafeHederaTokenService contract
// Informational
The configureAssets function in the AaveDistributionManager contract declares the parameter assetsConfigInput as memory. However, since this parameter is read-only during the function execution, using calldata instead of memory is more efficient. Calldata directly references the input data without copying it to memory, resulting in reduced gas consumption and improved performance.
Keeping the variable as calldata can optimize the function's performance and decrease transaction costs.
ACKNOWLEDGED: Changing assetsConfigInput to memory starts giving downstream compilation issues in StakedToken.sol and AaveIncentivesController.sol. So we are ignoring it.
// Informational
The _getAssetIndex function in the AaveDistributionManager contract currently takes four parameters: currentIndex, assetConfig, lastUpdateTimestamp, and totalBalance. However, assetConfig already contains the lastUpdateTimestamp and totalBalance data, making the other two parameters redundant. This redundancy leads to unnecessary gas consumption and inefficiency. Simplifying the function to accept only the composite parameter assetConfig can optimize gas usage and improve overall performance.
By eliminating the redundant parameters, the function becomes more efficient, reducing gas costs and enhancing performance. This approach ensures a more streamlined and cost-effective implementation.
SOLVED: The issue was solved by removing lastUpdateTimestamp.
Halborn used automated testing techniques to enhance the coverage of certain areas of the scoped contracts. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified all the contracts in the repository and was able to compile them correctly into their abi and binary formats, Slither was run on the all-scoped contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.
The reentrancy instances flagged by Slither were checked individually, and have been categorised as false positives.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed