In the addLiquidity() and the _chargeGreenFee() functions of the StrategyAerodromeGaugeGreenFee contract, there are several interactions with the Aerodrome router that allow for unlimited deadlines.

When interacting with external AMM protocols such as Aerodrome, it is recommended to avoid setting the deadline parameter to the current block.timestamp.

A malicious block builder can withhold swap transactions and execute them later when it's advantageous for manipulating the price or offloading tokens onto the user at a disadvantageous price. Implementing a deadline parameter restricts the time frame during which an attacker can carry out such exploits.

Code Location

function addLiquidity() internal {
    uint256 outputBal = IERC20(output).balanceOf(address(this));
    uint256 lp0Amt = outputBal / 2;
    uint256 lp1Amt = outputBal - lp0Amt;

    if (stable) {
        uint256 lp0Decimals = 10 ** IERC20Extended(lpToken0).decimals();
        uint256 lp1Decimals = 10 ** IERC20Extended(lpToken1).decimals();
        uint256 out0 = lpToken0 != output
            ? (ISolidlyRouter(unirouter).getAmountsOut(lp0Amt, outputToLp0Route)[outputToLp0Route.length] * 1e18) /
                lp0Decimals
            : lp0Amt;
        uint256 out1 = lpToken1 != output
            ? (ISolidlyRouter(unirouter).getAmountsOut(lp1Amt, outputToLp1Route)[outputToLp1Route.length] * 1e18) /
                lp1Decimals
            : lp1Amt;
        (uint256 amountA, uint256 amountB, ) = ISolidlyRouter(unirouter).quoteAddLiquidity(
            lpToken0,
            lpToken1,
            stable,
            factory,
            out0,
            out1
        );
        amountA = (amountA * 1e18) / lp0Decimals;
        amountB = (amountB * 1e18) / lp1Decimals;
        uint256 ratio = (((out0 * 1e18) / out1) * amountB) / amountA;
        lp0Amt = (outputBal * 1e18) / (ratio + 1e18);
        lp1Amt = outputBal - lp0Amt;
    }

    if (lpToken0 != output) {
        ISolidlyRouter(unirouter).swapExactTokensForTokens(
            lp0Amt,
            0,
            outputToLp0Route,
            address(this),
            block.timestamp
        );
    }

    if (lpToken1 != output) {
        ISolidlyRouter(unirouter).swapExactTokensForTokens(
            lp1Amt,
            0,
            outputToLp1Route,
            address(this),
            block.timestamp
        );
    }

    uint256 lp0Bal = IERC20(lpToken0).balanceOf(address(this));
    uint256 lp1Bal = IERC20(lpToken1).balanceOf(address(this));

    ISolidlyRouter(unirouter).addLiquidity(
        lpToken0,
        lpToken1,
        stable,
        lp0Bal,
        lp1Bal,
        1,
        1,
        address(this),
        block.timestamp
    );
}

function _chargeGreenFee() internal returns (uint256 _feeCharged) {
    uint256 outputBal = IERC20(output).balanceOf(address(this));
    (, , uint256 totalFeeInOutputToken) = getPerPoolGreenFee(outputBal);

    if (totalFeeInOutputToken == 0) {
        return 0;
    }

    address greenFeeToken = greenFeeConfig.greenFeeToken;

    uint256 greenFeeTokenBal;
    if (greenFeeToken != output) {  
        // swap output to greenFeeToken
        ISolidlyRouter(unirouter).swapExactTokensForTokens(
            totalFeeInOutputToken,
            0,
            outputToGreenFeeRoute,
            address(this),
            block.timestamp
        );
        greenFeeTokenBal = IERC20(greenFeeToken).balanceOf(address(this));
    } else {
        greenFeeTokenBal = totalFeeInOutputToken;
    }

    // Deposit all green token to green fee vault
    uint256 actualFeeDeposited = _depositGreenFee(greenFeeTokenBal);

    if (actualFeeDeposited != 0) {
        emit ContributedGreenFee(vault, actualFeeDeposited);
    }
}

Implement a more robust deadline parameter in order to restrict the time frame during which a trade can be executed.

RISK ACCEPTED: The KlimaDAO team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Code is part of fork.

Throughout the codebase, there are several roles that control critical contract configurations and operations. These roles have unrestricted power to modify core protocol parameters, pause functionality, upgrade contracts, and control user funds with no technical restrictions or safeguards in place. If these privileged roles are compromised or act maliciously, they could manipulate protocol parameters, halt operations, or withdraw user funds, effectively enabling rug pulls.

Particularly, the AutoRetireGreenVault, StrategyAerodromeGaugeGreen and GreenFeeManagerInitializable contracts inherit from the Ownable contract, which grants the owner unrestricted access to the contract's functions. The owner can change critical parameters, pause the contract, and withdraw funds from the contract without restrictions.

This centralization of power directly contradicts principles of decentralization and may put user deposits at significant risk.

Several remedial strategies can be employed, including but not limited to: transitioning control to a multi-signature wallet setup for critical functions, establishing community-driven governance for decision-making on fund management, and/or integrating time locks.

RISK ACCEPTED: The KlimaDAO team made a business decision to accept the risk of this finding and not alter the contracts.

The GreenFeeManagerInitializable contract and the StratFeeManagerInitializable inherits the OwnableUpgradeable contract implementation from OpenZeppelin's library and is used to restrict access to certain functions to the contract owner. The Ownable pattern allows the contract owner to transfer ownership to another address using the transferOwnership() function. However, the transferOwnership() function does not include a two-step process to transfer ownership.

In this regard, it is crucial that the address to which ownership is transferred is verified to be active and willing to assume ownership responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it.

Additionally, the renounceOwnership() function allows renouncing to the owner permission. Renouncing ownership before transferring it would result in the contract having no owner, eliminating the ability to call privileged functions.

Consider using OpenZeppelin's Ownable2StepUpgradeable contract over the OwnableUpgradeable implementation. Ownable2StepUpgradeable provides a two-step ownership transfer process, which adds an extra layer of security to prevent accidental ownership transfers.

Additionally, it is recommended that the owner cannot call the renounceOwnership() function to avoid losing ownership of the contract.

RISK ACCEPTED: The KlimaDAO team made a business decision to accept the risk of this finding and not alter the contracts.

The ratio calculation in the addLiquidity() function of the StrategyAerodromeGaugeGreen contract performs a division before multiplication. This could lead to an incorrect ratio calculation if precision loss occurs due to how Solidity handles division, rounding down to the nearest integer.

Code Location

uint256 ratio = (((out0 * 1e18) / out1) * amountB) / amountA;

It is recommended to perform the multiplication before the division to avoid precision loss. For example, an equivalent calculation would be:

uint256 ratio = (out0 * 1e18 * amountB) / (out1 * amountA);

SOLVED: The KlimaDAO team solved this finding in commit 619a499 by following the mentioned recommendation.

The _harvest() function of the StrategyAerodromeGaugeGreen contract makes several external calls for compounding earnings and charging performance fees before updating state. Although all external calls are made to trusted contracts, the sequence of getReward -> _chargeGreenFee -> addLiquidity -> deposit could leave the contract vulnerable to reentrancy. It is always recommended to implement the Checks-Effects-Interactions pattern to prevent unwanted reentrancy behavior.

Code Location

function _harvest(address callFeeRecipient) internal whenNotPaused {
    IAerodromeGauge(gauge).getReward(address(this));
    uint256 outputBal = IERC20(output).balanceOf(address(this));
    if (outputBal > 0) {
        // Given the output Bal is split into 1:1 ratio,
        // therefore we can charger the green fee on the outputBal
        // as a whole and then split the amounts into 1:1 ratio and add
        // liquidity to the AMM
        _chargeGreenFee();
        addLiquidity();

        uint256 wantHarvested = balanceOfWant();

        deposit();

        lastHarvest = block.timestamp;
        emit StratHarvest(msg.sender, wantHarvested, balanceOf());
    }
}

Implement the Checks-Effects-Interactions pattern or use a reentrancy guard mechanism such as OpenZeppelin's nonReentrant modifier from the ReentrancyGuardUpgradeable contract to prevent reentrancy behavior.

RISK ACCEPTED: The KlimaDAO team made a business decision to accept the risk of this finding and not alter the contracts, stating:

The likelihood of a reenterancy attack for the Harvest Function is negligible because 1. callFeeRecipient is not being used in the internal function and hence chances of it being involved in re-enterancy in this regard is none. 2. Another external call would be to Gauge. Now setting gauge value is an owner action and hence, likelihood of reenterancy is subdued.

Throughout the StrategyAerodromeGaugeGreen contract, there is consistent use of the SafeERC20 library to handle edge cases on ERC20 token that may no be fully compliant. However, in the retireStrat() function, the ERC20 token transfer is performed using the transfer() function directly.

This may lead to potential unexpected behavior if the token being transferred is not fully ERC20 compliant.

Code Location

function retireStrat() external {
    require(msg.sender == vault, "!vault");

    IAerodromeGauge(gauge).withdraw(balanceOfPool());

    uint256 wantBal = IERC20(want).balanceOf(address(this));
    IERC20(want).transfer(vault, wantBal);
}

It is recommended to use the safeTransfer() function from the SafeERC20 library to handle the transfer of ERC20 tokens in the retireStrat() function.

RISK ACCEPTED: The KlimaDAO team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Code is part of fork.

Throughout the codebase, there are several instances where an input address is assigned to a state variable without checking if the input address is address(0). Assigning address(0) to a state variable can lead to unexpected behavior of the system. Instances of this issue include:

• StrategyAerodromeGaugeGreen.initialize()

• StratFeeManagerInitializable.setVault()

• StratFeeManagerInitializable.setUnirouter()

• StratFeeManagerInitializable.setKeeper()

• StratFeeManagerInitializable.setStrategist()

• StratFeeManagerInitializable.setBeefyFeeRecipient()

• StratFeeManagerInitializable.setBeefyFeeConfig()

It is recommended to check if any input address is the address(0) before assigning it to a state variable to prevent unexpected behavior.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the codebase, several OpenZeppelin contracts implementations are inherited and used. These contracts are used to implement access control, pausing functionality, ERC20 token standards, and more. However, some of the versions of these contracts are outdated and may contain vulnerabilities that may have been fixed in newer versions.

For more reference about OpenZeppelin contracts versions and their vulnerabilities, see https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts

Consider updating all OpenZeppelin contracts to the latest versions to benefit from the latest security patches and improvements.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts, stating:

Part of Fork and it's a bigger undertaking to update to upgraded versions.

The ratio calculation in the addLiquidity() function of the StrategyAerodromeGaugeGreen contract performs two divisions without explictly ensuring that none of the denominators are 0. This could lead to a division by zero type error. For example, this scenario could appear if the getAmountsOut() function of the Aerodrome router returns 0.

It is recommended to add an explicit check to ensure that the denominator is not zero before performing any division.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts, stating:

Part of Fork + We don't plan to onboard stable liquidity.

The deposit() function in the StrategyAerodromeGaugeGreen contract allows for depositing all current want tokens to the Aerodrome Gauge external contract. However, this function can be triggered by anyone, which could lead to unexpected behavior if the correct execution flow is expected to deposit the funds in the harvest() function only:

function deposit() public whenNotPaused {
    uint256 wantBal = IERC20(want).balanceOf(address(this));

    if (wantBal > 0) {
        IAerodromeGauge(gauge).deposit(wantBal, address(this));
        emit Deposit(balanceOf());
    }
}

Consider adding access control to the deposit() function to allow only specific addresses to trigger the function, for example the Vault contract. Additionally, document exhaustively the expected behavior of the function and the expected execution flow.

PARTIALLY SOLVED: The KlimaDAO team partially solved this finding in commit 6cf6ace by adding information to the deposit() NASTPEC comments explaining its functionality.

The harvest() mechanism in the StrategyAerodromeGaugeGreen contract allows to compound earnings and charges performance fee. However, there is no minimum harvest threshold, which could lead to inefficient harvesting when triggered with low amounts.

Additionally, the callFeeRecipient parameter is not used in the function, which may indicate a potential inconsistency in the function's design and lead to confusion since both the harvest() and harvest(address callFeeRecipient) functions will provide the same functionality.

Code Location

function _harvest(address callFeeRecipient) internal whenNotPaused {
    IAerodromeGauge(gauge).getReward(address(this));
    uint256 outputBal = IERC20(output).balanceOf(address(this));
    if (outputBal > 0) {
        // Given the output Bal is split into 1:1 ratio,
        // therefore we can charger the green fee on the outputBal
        // as a whole and then split the amounts into 1:1 ratio and add
        // liquidity to the AMM
        _chargeGreenFee();
        addLiquidity();

        uint256 wantHarvested = balanceOfWant();

        deposit();

        lastHarvest = block.timestamp;
        emit StratHarvest(msg.sender, wantHarvested, balanceOf());
    }
}

function harvest() external virtual {
    _harvest(tx.origin);
}

function harvest(address callFeeRecipient) external virtual {
    _harvest(callFeeRecipient);
}

Consider adding a minimum harvest threshold to the harvest() function to prevent inefficient harvesting. Additionally, ensure that the callFeeRecipient parameter is used in the function to avoid confusion and maintain consistency in the function's design, or remove it if it is not necessary.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts, stating:

Part of Fork: Acknowledge the capital inefficiency, but it only gets triggered when there's not enough daily rewards, or the pool size is small.

Several functions in the StrategyAerodromeGaugeGreen contract make external calls that return values, but the return values are ignored. Ignoring return values can lead to unexpected behavior if the return values are used to determine the success or failure of the external calls. Instances of this issue are in:

• StrategyAerodromeGaugeGreen._harvest()

• StrategyAerodromeGaugeGreen._chargeGreenFee()

• StrategyAerodromeGaugeGreen.addLiquidity()

Ensure that the return values of external calls are handled appropriately throughout the contracts.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.

In the StrategyAerodromeGaugeGreenFee contract, there are several instances where magic numbers are used. Magic numbers are direct numerical or string values used in the code without any explanation or context. This practice can lead to code maintainability issues, potential errors, and difficulty in understanding the code's logic.

To improve the code’s readability and facilitate refactoring, consider defining a constant for every magic number, giving it a clear and self-explanatory name. Consider adding an inline comment explaining how the magic numbers are calculated or why they are chosen for complex values.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.

In the StrategyAerodromeGaugeGreen contract, the IInterchainAutoRetireGreenVault interface is imported but never used. Additionally, the _checkFeeManager() function in the GreenFeeManagerInitializable contract is not used in the contract, as well as some errors in the GreenVaultErrorsLib are declared but not used in the AutoRetireGreenVault contract. Unused imports can be removed to improve code readability and maintainability.

Consider removing unused imports, functions and errors to declutter the code and improve readability or implement the intended functionality.

SOLVED: The KlimaDAO team solved this finding in commit 6cf6ace by following the mentioned recommendation.

Some functions in the StrategyAerodromeGaugeGreen contract are currently defined with the public visibility modifier, even though the functions are not called from within the contract, resulting in higher gas costs than necessary. Instances of this issue include:

• StrategyAerodromeGaugeGreen.callReward()

• StrategyAerodromeGaugeGreen.panic()

Modify the public functions not used within the contracts with the external visibility modifier.

SOLVED: The KlimaDAO team solved this finding in commit c3e53e6 by following the mentioned recommendation.

The contracts in scope currently use floating pragma versions ^0.8.0 and ^0.8.23 which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.0, or 0.8.23 respectively, and less than 0.9.0.

However, 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 to the same version used during development and testing.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the file in scope, there are several instances of use of revert strings over custom errors.

In Solidity 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.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

For more reference, see here.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.

NATSPEC comments are essential for providing clear and concise explanations of the purpose and functionality of the code. They help developers understand the codebase, facilitate code maintenance, and improve overall code quality. However, throughout the contracts in scope, not all the functions and declared variables have clear NATSPEC documentation.

Particularly, the StrategyAerodromeGaugeGreenFee contract is missing NATSPEC comments for the functions and declared variables.

Add NATSPEC comments to all functions and declared variables in the contracts to provide clear explanations of their purpose, inputs, outputs, and any other relevant information. This will help improve code readability, maintainability, and overall quality.

ACKNOWLEDGED: The KlimaDAO team made a business decision to acknowledge this finding and not alter the contracts.