Staking-Module - Bonzo

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.

Remediation Plan

SOLVED: The Bonzo team solved this issue as recommended. The REWARDS_VAULT will require to approve AaveIncentivesController to spend the _whbarContract tokens.

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.

Remediation Plan

SOLVED: The Bonzo team solved this issue as recommended.

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.

Remediation Plan

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.

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:

• MAINNET: 0x0000000000000000000000000000000000163b59

• TESTNET: 0x0000000000000000000000000000000000003ad1

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.

Remediation Plan

SOLVED: The Bonzo team solved this issue as recommended.

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.

Remediation Plan

SOLVED: The Bonzo team solved this issue as recommended.

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.

Remediation Plan

SOLVED: The Bonzo team solved this issue as recommended. The IStakedToken was removed.

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.

Remediation Plan

ACKNOWLEDGED: Ignoring as it causes compatibility issues with the older versions of the SafeHederaTokenService contract

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.

Remediation Plan

ACKNOWLEDGED: Changing assetsConfigInput to memory starts giving downstream compilation issues in StakedToken.sol and AaveIncentivesController.sol. So we are ignoring it.

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.

Remediation Plan

SOLVED: The issue was solved by removing lastUpdateTimestamp.