Earn V1 - Concrete

There's a logic issue on the WithdrawaQueue contract. The problem arises when the vault wanted to finalize a request withdrawal ID before preparing a withdrawal with a request ID-1.

/// @dev preapares a request to be transferred
    ///  Emits WithdrawalClaimed event
    //TODO test this function
    //-next-line naming-convention
    function prepareWithdrawal(uint256 _requestId, uint256 _avaliableAssets)
        external
        onlyOwner
        returns (address recipient, uint256 amount, uint256 avaliableAssets)
    {
        console.log("LAST FINALIZED REQUEST ID: ",lastFinalizedRequestId);
        console.log("_requestId: ",_requestId);
        if (_requestId == 0) revert InvalidRequestId(_requestId);
        if (_requestId < lastFinalizedRequestId) revert RequestNotFoundOrNotFinalized(_requestId);

        WithdrawalRequest storage request = _requests[_requestId];

        if (request.claimed) revert RequestAlreadyClaimed(_requestId);

        recipient = request.recipient;

        WithdrawalRequest storage prevRequest = _requests[_requestId - 1];

        amount = request.cumulativeAmount - prevRequest.cumulativeAmount;
        console.log("amount: prepare withdrawal: ",amount);
        console.log("_avaliableAssets: ",_avaliableAssets);

        if (_avaliableAssets > amount) {
            assert(_requestsByOwner[recipient].remove(_requestId));
            avaliableAssets = _avaliableAssets - amount;
            request.claimed = true;
            //This is commented to fit the requirements of the vault
            //instead of this we will call _withdrawStrategyFunds
            //IERC20(TOKEN).safeTransfer(recipient, realAmount);

            emit WithdrawalClaimed(_requestId, recipient, amount);
        }
    }

Since the owner is the vault and the only way the vault can call  _finalize is through  batchClaimWithdrawal, the issue has been downgraded from high to low due to the protocol design. However, be sure before production or future releases that the _finalize external function is not called by the vault for security reasons.

The batchClaimWithdrawal function, is calling first the prepareWithdrawal (claimWithdrawal private function) instead of _finalize.

/**
     * @notice Claims multiple withdrawal requests starting from the lasFinalizedRequestId.
     * @dev This function allows the contract owner to claim multiple withdrawal requests in batches.
     * @param maxRequests The maximum number of withdrawal requests to be processed in this batch.
     */
    function batchClaimWithdrawal(uint256 maxRequests) external onlyOwner nonReentrant {
        if (address(withdrawalQueue) == address(0)) revert QueueNotSet();
        uint256 availableAssets = getAvailableAssetsForWithdrawal();

        uint256 lastFinalizedId = withdrawalQueue.getLastFinalizedRequestId();
        uint256 lastCreatedId = withdrawalQueue.getLastRequestId();
        uint256 newLastFinalized = lastFinalizedId; 

        uint256 max = lastCreatedId < lastFinalizedId + maxRequests ? lastCreatedId : lastFinalizedId + maxRequests;
        console.log("MAX: ",max);

        for (uint256 i = lastFinalizedId + 1; i <= max;) {
            uint256 newAvailiableAssets = claimWithdrawal(i, availableAssets);
            // -next-line incorrect-equality
            if (newAvailiableAssets == availableAssets) break;

            availableAssets = newAvailiableAssets;
            newLastFinalized = i;
            unchecked {
                i++;
            }
        }

        if (newLastFinalized != lastFinalizedId) {
            withdrawalQueue._finalize(newLastFinalized);
        }
    }

Consider ensuring as internal the _finalize function.

RISK ACCEPTED: The Concrete team accepted the risk of this finding. They will not make the function internal. The vault doesn't have any way to directly call that function. The only way to do it is through impersonation using Foundry or Hardhat.

ERC20(token).approve reverted if the underlying ERC20 token approve did not return boolean. When transferring the token, the protocol uses safeTransfer and safeTransferFrom, but when approving the payout token, the safeApprove is not used for non-standard token such as USDT, calling approve will revert because the OpenZeppeling ERC20 enforces the underlying token return a boolean.

function approve(address spender, uint256 value) public virtual returns (bool) {
        address owner = _msgSender();
        _approve(owner, spender, value);
        return true;
    }

While the token such as USDT does not return boolean: https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L126
USDT or other ERC20 token that does not return boolean for approve is not supported as the payout token. It has been observed on the SiloV1Strategy contract, missing require on the approve function.

baseAsset_.approve(address(silo), type(uint256).max);

Consider using safeApprove instead of approve.

SOLVED: The Concrete team solved the issue by applying recommendations.

There is a potential denial of service (DoS) vulnerability in the _getStrategy function. The vaults array, which stores all vaults created for a specific token, lacks an upper limit on its size. Consequently, the contract owner could theoretically create an infinite number of vaults. This array retrieves all vaults associated with a particular token, which means an infinite number of vaults could be created for a given token. This would cause a denial of service when the _getStrategy function is called due to the excessive size of the vaults array.

function _getStrategy(address tokenAddress, uint256 amount) internal returns (address, bool) {
        //retrieves all vaults created for an specific token

        address[] memory vaults = vaultRegistry.getVaultsByToken(tokenAddress);
        //TODO change to a criteria where when can decidede base on yield

        address selectedProtectionStrat = address(0x0);
        bool requiresFunds = true;
        //Iterates over array of vaults to find the best vault to withdraw from
        uint256 len = vaults.length;
        uint256 maxYieldFound = type(uint256).max;
        for (uint256 i; i < len; i++) {
            IConcreteMultiStrategyVault currentVault = IConcreteMultiStrategyVault(vaults[i]);
            address protectionStrat = currentVault.protectStrategy();
            //only considers vaults with protection strategies
            if (protectionStrat == address(0x0)) {
                continue;
            }

            uint256 protectionStratYield = IMockStrategy(protectionStrat).highWatermark();

            //if the protection strategy has enough assets to withdraw we are done with the search
            if (
                IMockStrategy(protectionStrat).getAvailableAssetsForWithdrawal() >= amount
                    && (protectionStratYield < maxYieldFound || requiresFunds == true)
            ) {
                console.log("hola puto");
                selectedProtectionStrat = protectionStrat;
                requiresFunds = false;
                maxYieldFound = protectionStratYield;
            }
            if (requiresFunds == false) {
                continue;
            }
            

            if (currentVault.getAvailableAssetsForWithdrawal() >= amount && protectionStratYield < maxYieldFound) {
                selectedProtectionStrat = protectionStrat;
                requiresFunds = true;
                maxYieldFound = protectionStratYield;
            }
        }
        return (selectedProtectionStrat, requiresFunds);
    }

Gas Consumption Concern

Additionally, the getVaultsByToken function consumes a significant amount of gas when handling a large number of vaults. This issue is particularly evident if an unusually large number of vaults are created for a specific token (as demonstrated in the proof of concept).

function getVaultsByToken(address asset) external view virtual returns (address[] memory vaults) {
        return vaultsByToken[asset].values();
    }

Irreversibility of the Operation

The operation is irreversible for the affected token. Once a vast number of vaults are created for a token, all subsequent operations on vaults associated with that token will fail. Given that the operation requires privileged access, the severity of this issue has been downgraded to low.

1. Implement a limit on the number of vaults that can be created for each token.

2. Optimize the getVaultsByToken function to handle large arrays more efficiently.

3. Consider adding mechanisms to reverse or mitigate the creation of excessive vaults.

By addressing these recommendations, the potential for a denial of service can be significantly reduced, ensuring the robustness and reliability of the contract.

SOLVED: The Concrete team solved this issue.

It has been observed that the user blueprint did not receive the asset tokens, since the executeBorrowClaim function is executed without code.

/// @notice Function to request assets from the vault.
    /// @dev Requests assets from the vault and executes a borrow claim through the protection strategy.
    /// @param tokenAddress The address of the token.
    /// @param amount_ The amount of assets to request.
    /// @param userBlueprint The address of the user's blueprint contract.
    function requestToken(VaultFlags, address tokenAddress, uint256 amount_, address payable userBlueprint)
        external
        onlyBlueprint
    {
        uint256 amount = amount_;
        (address protectionStrat, bool requiresFunds) = _getStrategy(tokenAddress, amount);
        console.log(protectionStrat);
        if (protectionStrat == address(0x0)) {
            //iterates over array
            uint256 len = tokenCascade.length;
            for (uint256 i; i < len; i++) {
                //We avoid using the same token as the one that failed
                if (tokenAddress == tokenCascade[i]) continue;

                //TODO change amount

                amount = _convertFromTokenToStable(tokenAddress, amount_);

                //We control both the length of the array and the external call
                // slither-disable-next-line calls-loop
                (protectionStrat, requiresFunds) = _getStrategy(tokenCascade[i], amount);
                if (protectionStrat != address(0x0)) {
                    break;
                }
            }
        }

        if (protectionStrat == address(0x0)) {
            revert NoProtectionStrategiesFound();
        }
        emit ClaimRequested(protectionStrat, amount, IMockProtectStrategy(protectionStrat).asset(), userBlueprint);
        IMockProtectStrategy(protectionStrat).executeBorrowClaim(amount, userBlueprint);
    }

MockERC44656Protect.sol:

function executeBorrowClaim(uint256 amount, address recipient) external {
        console.log("Entered to empty function from on scope contract");
        //empty
        
    }

Since it's a mock, the issue has been downgraded, however it's not recommended using a mock contract within a contract. If for somehow this is not updated, then the wrong function will be executed with unexpected errors or behaviors for the users using the money printer.

Consider changing the proper interface IProtectStrategy instead of IMockProtectStrategy on the scope contract ClaimRouter due to could lead to unexpected behavior since the critical code inside executeBorrowClaim is never run.

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

Ownership of the contracts can be lost as the RewardManager,TokenRegistry,ImplementationRegistry,DeploymentManager and VaultFactory contract is inherited from the Ownable contract, and their ownership can be transferred in a single-step process. The address the ownership is changed to should be verified to be active or willing to act as the owner.

function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _transferOwnership(newOwner);
}

function _transferOwnership(address newOwner) internal virtual {
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
}

Consider using the Ownable2Step https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol library over the Ownable library.

ACKNOWLEDGED : The Concrete team acknowledged the issue. Since each of the contracts is owned by other contracts, the ability to transfer or renounce ownership is significantly limited. The use of Ownable2Step requires that the new owner accept ownership. Without a significant re-write of the existing contracts (which would negate the functionality of Ownable2Step) this is not feasible.

Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The Openzeppelin’s Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));

To mitigate the risks associated with unintended or unauthorized ownership transfers, it is recommended to adopt a more secure ownership transfer mechanism, such as OpenZeppelin's Ownable2Step. This implementation requires a two-step process for ownership transfer: first, the current owner initiates the transfer by specifying a new owner, and then the new owner must accept the ownership. This process adds an additional layer of security by ensuring that ownership is only transferred after explicit confirmation from both parties involved.

Implementing Ownable2Step will not only enhance the security of the contracts by preventing accidental or malicious ownership transfers but also ensure that any systems or contracts relying on its authorization remain consistent and up-to-date. This recommendation aligns with best practices for contract ownership management and governance within the Ethereum ecosystem.

ACKNOWLEDGED : The Concrete team acknowledged the issue. Since each of the contracts is owned by other contracts, the ability to transfer or renounce ownership is significantly limited. The use of Ownable2Step requires that the new owner accept ownership. Without a significant re-write of the existing contracts (which would negate the functionality of Ownable2Step) this is not feasible.

The ability to pauseVault() and unpauseVault() the protocol is unilaterally controlled by a single admin account, which if compromised could be exploited to attempt to lock up the protocol and extort users.

It is advised that these capabilities are separated into distinct roles in an effort to reduce the consolidation of power that could be wielded in the event of a protocol exploit.

Consider adopting both an UNPAUSE_ROLE and PAUSE_ROLE.

ACKNOWLEDGED : The Concrete team acknowledged the issue. The client belief that the likelihood of this is exceptionally low. The admin role will be granted to a multi-sig to prevent rogue action. If they were to introduce a pause and unpause role, the same multisig would be provided in both roles, negating any positive benefits and creating unnecessary overhead.

It has been observed that if a particular vault wanted to be removed, the _handleRemoveVault function iterates over all vaultArray consuming a lot of gas in case there's a huge amount of vault created on the array vaultArray_. If the array is big enough, the block gas limit will be reached and the transaction will never get processed. For that reason, it is recommended to add a require statement for limiting adding, for example, more than n different vault.

 function _handleRemoveVault(address vault_, address[] storage vaultArray_) internal {
        uint256 length = vaultArray_.length;
        for (uint256 i = 0; i < length;) {
            if (vaultArray_[i] == vault_) {
                if (i < length - 1) {
                    vaultArray_[i] = vaultArray_[length - 1];
                }
                vaultArray_.pop();
                break;
            }
            unchecked {
                i++;
            }
        }
    }

Consider ensuring a fixed created vaults in order to avoid potential denial of service on _handleRemoveVault function.

SOLVED: The Concrete team solved this issue.

Most of the solidity for loops use an uint256 variable counter that increments by 1 and starts at 0. These increments don't need to be checked for over/underflow because the variable will never reach the max capacity of uint256 as it would run out of gas long before that happens.

It is recommended to uncheck the increments in for loops to save gas. For example, instead of:

for (uint256 i; i < len; i++) {
                Strategy memory strategy = strategies[i];
                //We control both the length of the array and the external call
                //-next-line calls-loop
                uint256 withdrawable = strategy.strategy.previewRedeem(strategy.strategy.balanceOf(address(this)));
                if (diff.mulDiv(strategy.allocation.amount, 10_000, Math.Rounding.Ceil) > withdrawable) {
                    revert InsufficientFunds(strategy.strategy, diff * strategy.allocation.amount, withdrawable);
                }
                uint256 amountToWithdraw = amount_.mulDiv(strategy.allocation.amount, 10_000, Math.Rounding.Ceil);
                //We control both the length of the array and the external call
                //-next-line unused-return,calls-loop
                strategy.strategy.withdraw(amountToWithdraw, receiver_, address(this));
                totalWithdrawn += amountToWithdraw;
            }

Use:

for (uint256 i; i < len;) {
                Strategy memory strategy = strategies[i];
                //We control both the length of the array and the external call
                //-next-line calls-loop
                uint256 withdrawable = strategy.strategy.previewRedeem(strategy.strategy.balanceOf(address(this)));
                if (diff.mulDiv(strategy.allocation.amount, 10_000, Math.Rounding.Ceil) > withdrawable) {
                    revert InsufficientFunds(strategy.strategy, diff * strategy.allocation.amount, withdrawable);
                }
                uint256 amountToWithdraw = amount_.mulDiv(strategy.allocation.amount, 10_000, Math.Rounding.Ceil);
                //We control both the length of the array and the external call
                //-next-line unused-return,calls-loop
                strategy.strategy.withdraw(amountToWithdraw, receiver_, address(this));
                totalWithdrawn += amountToWithdraw;
                unchecked {++i};
            }

SOLVED : The Concrete team solved the issue by adding unchecked statement.

The function retireStrategy calls _protocolWithdraw(balanceOfUnderlying(shares), 0) without verifying that balanceOfUnderlying returns a valid value. This could potentially cause issues if balanceOfUnderlying does not return the expected value.

function retireStrategy() external onlyOwner {
    _getRewardsToStrategy();
    uint256 shares = collateralToken.balanceOf(address(this));
    _protocolWithdraw(balanceOfUnderlying(shares), 0);
}

Consider adding proper checks on the return of balanceOfUnderlying function.

SOLVED: The Concrete team solved the issue by applying recommendations.

The functions pullFundsFromSingleStrategy, pushFundsIntoSingleStrategy, and addStrategy fail to implement checks for out-of-bounds access on the array of strategies with which the vault can interact.

/**
     * @notice Pulls funds back from a single strategy into the vault.
     * @dev Can only be called by the vault owner.
     * @param index_ The index of the strategy from which to pull funds.
     */
    function pullFundsFromSingleStrategy(uint256 index_) external onlyOwner {
        Strategy memory strategy = strategies[index_];
        // slither-disable-next-line unused-return
        strategy.strategy.redeem(strategy.strategy.balanceOf(address(this)), address(this), address(this));
        if (!IERC20(asset()).approve(address(strategy.strategy), 0)) revert ERC20ApproveFail();
    }

    /**
     * @notice Pushes funds from the vault into a single strategy based on its allocation.
     * @dev Can only be called by the vault owner. Reverts if the vault is idle.
     * @param index_ The index of the strategy into which to push funds.
     */
    function pushFundsIntoSingleStrategy(uint256 index_) external onlyOwner {
        if (vaultIdle) revert VaultIsIdle();
        Strategy memory strategy = strategies[index_];
        // slither-disable-next-line unused-return
        strategies[index_].strategy.deposit(
            totalAssets().mulDiv(strategy.allocation.amount, 10_000, Math.Rounding.Floor), address(this)
        );
    }

When the replace_ parameter is set to true, the function did not perform a check on the index.

function addStrategy(uint256 index_, bool replace_, Strategy calldata newStrategy_)
        external
        nonReentrant
        onlyOwner
        takeFees
    {
        //Ensure that allotments do not total > 100%
        uint256 allotmentTotals = 0;
        uint256 len = strategies.length;
        for (uint256 i; i < len; i++) {
            allotmentTotals += strategies[i].allocation.amount;
        }

        if (replace_) {
            if (allotmentTotals - strategies[index_].allocation.amount + newStrategy_.allocation.amount > 10000) {
                revert AllotmentTotalTooHigh();
            }
            // slither-disable-next-line unused-return
            strategies[index_].strategy.redeem(
                strategies[index_].strategy.balanceOf(address(this)), address(this), address(this)
            );
            if (!IERC20(asset()).approve(address(strategies[index_].strategy), 0)) revert ERC20ApproveFail();

            emit StrategyReplaced(strategies[index_], newStrategy_);

            strategies[index_] = newStrategy_;
            if (!IERC20(asset()).approve(address(newStrategy_.strategy), type(uint256).max)) revert ERC20ApproveFail();
        } else {
            if (allotmentTotals + newStrategy_.allocation.amount > 10000) {
                revert AllotmentTotalTooHigh();
            }
            strategies.push(newStrategy_);
            if (!IERC20(asset()).approve(address(newStrategy_.strategy), type(uint256).max)) revert ERC20ApproveFail();
        }
        emit StrategyAdded(newStrategy_);
    }

It is advisable to implement out-of-bounds checks when accessing arrays in Solidity. Additionally, providing an error message when attempting to fetch data beyond the array's limit can inform the user about the cause of the execution reversion.

SOLVED : The Concrete team solved the issue by adding array index checks.

The following methods are public and could be external. External is more gas optimize that public, so it should be used as much as possible.

• swapTokensForReward() should be declared external:

  • Swapper.swapTokensForReward() (swapper/Swapper.sol#75-94).

• setRewardManager() should be declared external:

  • Swapper.setRewardManager() (swapper/Swapper.sol#102-105).

• disableTokenForSwap() should be declared external:

  • Swapper.disableTokenForSwap() (swapper/Swapper.sol#111-113).

Use external instead of public methods.

SOLVED : The Concrete team solved the issue by replacing public to external.

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing smart contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

ACKNOWLEDGED : The Concrete team acknowledged the issue. It is their stance that this a low probability over a given time horizon. Additionally, if the client elects to deploy to a chain that is incompatible with Shanghai, it can be created a contract that utilizes an older Solidity version.

Some missing zero addresses have been found:

1. The addVault function allows the addition of a vault with a zero address.

2. The strategy struct permits passing a zero address during its initialization.

function addVault(address vault_, bytes32 vaultId_) external override onlyOwner {
        if (vaultExists[vault_]) {
            revert VaultAlreadyExists();
        }
        vaultExists[vault_] = true;
        vaultIdToAddressArray[vaultId_].push(vault_);
        allVaultsCreated.push(vault_);
        assert(vaultsByToken[IERC4626(vault_).asset()].add(vault_));
        console.log("VAULT ADDED: ",vault_);
        emit VaultAdded(vault_, vaultId_);
    }

function initialize(
        IERC20 baseAsset_,
        string memory shareName_,
        string memory shareSymbol_,
        Strategy[] memory strategies_,
        address feeRecipient_,
        VaultFees memory fees_,
        uint256 depositLimit_,
        address owner_
    ) external initializer nonReentrant {
        __ERC4626_init(IERC20Metadata(address(baseAsset_)));
        __ERC20_init(shareName_, shareSymbol_);
        __Ownable_init(owner_);
        _queue_init();
        if (address(baseAsset_) == address(0)) revert InvalidAssetAddress();

        // Loop through provided strategies, validate them, and approve the base asset for each.
        uint256 len = strategies_.length;
        //console.log("Strategies len: ",len);
        for (uint256 i; i < len;) {
            //We control both the length of the array and the external call
            // slither-disable-next-line calls-loop
            if (strategies_[i].strategy.asset() != address(baseAsset_)) {
                revert VaultAssetMismatch();
            }
            //console.log("Protection strats on concret init: ",strategies_[i].strategy.isProtectStrategy());
            if (strategies_[i].strategy.isProtectStrategy()) {
                if (protectStrategy != address(0x0)) revert MultipleProtectStrat();
                protectStrategy = address(strategies_[i].strategy);
            }
            strategies.push(strategies_[i]);
            //We control both the length of the array and the external call
            // slither-disable-next-line calls-loop
            if (!baseAsset_.approve(address(strategies_[i].strategy), type(uint256).max)) {
                revert ERC20ApproveFail();
            }
            unchecked {
                i++;
            }
        }

To address the identified issues of allowing zero addresses in the addVault and initialize functions, you need to add checks to ensure that zero addresses are not passed for vault_ in addVault and for strategy and other relevant addresses in initialize.

ACKNOWLEDGED: The Concrete team acknowledged this finding.

Most of the solidity for loops use an uint256 variable counter that increments by 1 and starts at 0. These increments don't need to be checked for over/underflow because the variable will never reach the max capacity of uint256 as it would run out of gas long before that happens.

It is recommended to uncheck the increments in for loops to save gas. For example, instead of:

 for (uint256 i; i < vaults.length; i++) {
            address protectionStrat = IConcreteMultiStrategyVault(vaults[i]).protectStrategy();

            //only considers vaults with protection strategies
            if (protectionStrat == address(0x0)) {
                continue;
            }

            lastProtectionStrat = protectionStrat;
            uint256 stratBorrowDebt = IMockProtectStrategy(protectionStrat).getBorrowDebt();

            if (stratBorrowDebt == 0) continue;

            uint256 amountToBeSent = amount_.mulDiv(stratBorrowDebt, totalBorrowDebt, Math.Rounding.Floor);

            //this function is only called by the blueprint so we can trust the userBlueprint
            totalSent += amountToBeSent;
            //TODO HAndle cases where the amount sent is more the one expected
            //the funtion should be able to set the rest as rewards in the case of the reapyment exceding the debt

            //slither-disable-next-line arbitrary-send-erc20
            IERC20(tokenAddress).safeTransferFrom(userBlueprint, protectionStrat, amountToBeSent);
            if (isReward) {
                emit RewardAdded(protectionStrat, amountToBeSent);
                continue;
            }

            if (amountToBeSent > stratBorrowDebt) {
                emit RewardAdded(protectionStrat, amountToBeSent - stratBorrowDebt);
                emit Repayment(protectionStrat, stratBorrowDebt);
                IMockProtectStrategy(protectionStrat).updateBorrowDebt(stratBorrowDebt);
            } else {
                emit Repayment(protectionStrat, amountToBeSent);
                IMockProtectStrategy(protectionStrat).updateBorrowDebt(amountToBeSent);
            }
        }

Use:

for (uint256 i; i < vaults.length; ) {
            address protectionStrat = IConcreteMultiStrategyVault(vaults[i]).protectStrategy();

            //only considers vaults with protection strategies
            if (protectionStrat == address(0x0)) {
                continue;
            }

            lastProtectionStrat = protectionStrat;
            uint256 stratBorrowDebt = IMockProtectStrategy(protectionStrat).getBorrowDebt();

            if (stratBorrowDebt == 0) continue;

            uint256 amountToBeSent = amount_.mulDiv(stratBorrowDebt, totalBorrowDebt, Math.Rounding.Floor);

            //this function is only called by the blueprint so we can trust the userBlueprint
            totalSent += amountToBeSent;
            //TODO HAndle cases where the amount sent is more the one expected
            //the funtion should be able to set the rest as rewards in the case of the reapyment exceding the debt

            //slither-disable-next-line arbitrary-send-erc20
            IERC20(tokenAddress).safeTransferFrom(userBlueprint, protectionStrat, amountToBeSent);
            if (isReward) {
                emit RewardAdded(protectionStrat, amountToBeSent);
                continue;
            }

            if (amountToBeSent > stratBorrowDebt) {
                emit RewardAdded(protectionStrat, amountToBeSent - stratBorrowDebt);
                emit Repayment(protectionStrat, stratBorrowDebt);
                IMockProtectStrategy(protectionStrat).updateBorrowDebt(stratBorrowDebt);
            } else {
                emit Repayment(protectionStrat, amountToBeSent);
                IMockProtectStrategy(protectionStrat).updateBorrowDebt(amountToBeSent);
            }
         unchecked {++i};
        }

ACKNOWLEDGED: The Concrete team acknowledged this finding.

It has been observed that no parameter passed into the constructor was properly validated.

constructor(
        IERC20 baseAsset_,
        address feeRecipient_,
        address owner_,
        uint256 rewardFee_,
        address claimRouter_,
        address vault_
    ) {
        IERC20Metadata metaERC20 = IERC20Metadata(address(baseAsset_));

        rewardFee = rewardFee_;
        //This can be initially set to zero and then updated by the owner
        //slither-disable-next-line missing-zero-check
        claimRouter = claimRouter_;

        RewardToken[] memory rewardTokenEmptyArray = new RewardToken[](0);
        __StrategyBase_init(
            baseAsset_,
            string.concat("Concrete Earn Protect ", metaERC20.symbol(), " Strategy"),
            string.concat("ctPct-", metaERC20.symbol()),
            feeRecipient_,
            type(uint256).max,
            owner_,
            rewardTokenEmptyArray,
            vault_
        );
        //slither-disable-next-line unused-return
        //baseAsset_.approve(claimRouter, type(uint256).max);
    }

The only parameter that can be set again is claimRouter_ through the setClaimRouter function by the owner.

Consider implementing proper validation for all parameters passed into the constructor. This will enhance the security and robustness of the contract by ensuring that invalid or malicious input cannot compromise its functionality. Specifically, consider adding checks to verify the validity of addresses, ensuring they are not zero addresses, and confirming that the rewardFee_ is within an acceptable range.

ACKNOWLEDGED: The Concrete team acknowledged this finding.

The SafeERC20 and Math libraries are imported twice on the StrategyBase contract. This should be cleaned up to avoid confusion.

import {
    ERC4626Upgradeable,
    ERC20Upgradeable as ERC20,
    IERC20,
    IERC20Metadata,
    IERC4626
} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol";
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {Context} from "@openzeppelin/contracts/utils/Context.sol";
import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {Errors} from "../interfaces/Errors.sol";
import {ReturnedRewards} from "../interfaces/IStrategy.sol";
import {
    VaultFees,
    VaultInitParams,
    Strategy,
    IConcreteMultiStrategyVault
} from "../interfaces/IConcreteMultiStrategyVault.sol";

Consider removing the duplicated imports.

SOLVED: The Concrete team solved the issue by applying recommendations.

Consider to add events for critical state-changing functions to improve traceability and debugging.

/**
     * @notice Sets the recipient address for protocol fees.
     * @dev Can only be called by the contract owner.
     * @param feeRecipient_ The address to which protocol fees will be sent.
     */
    function setFeeRecipient(address feeRecipient_) external onlyOwner {
        if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();
        feeRecipient = feeRecipient_;
    }

    /**
     * @notice Sets the maximum limit for deposits into the strategy.
     * @dev Can only be called by the contract owner.
     * @param depositLimit_ The maximum amount that can be deposited.
     */
    //TODO: Add events for these functions
    //-next-line events-maths
    function setDepositLimit(uint256 depositLimit_) external onlyOwner {
        if (depositLimit_ == 0) revert InvalidDepositLimit();
        depositLimit = depositLimit_;
    }

Consider adding events:

event FeeRecipientUpdated(address indexed newFeeRecipient);
event DepositLimitUpdated(uint256 newDepositLimit);

function setFeeRecipient(address feeRecipient_) external onlyOwner {
    if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();
    feeRecipient = feeRecipient_;
    emit FeeRecipientUpdated(feeRecipient_);
}

function setDepositLimit(uint256 depositLimit_) external onlyOwner {
    if (depositLimit_ == 0) revert InvalidDepositLimit();
    depositLimit = depositLimit_;
    emit DepositLimitUpdated(depositLimit_);
}

ACKNOWLEDGED: The Concrete team acknowledged this finding.

The requestFunds function is marked with onlyProtect, but there's no mechanism to change or update the protectStrategy address once it is set. This could be a problem if the protectStrategy needs to be updated or changed due to unforeseen issues or upgrades.

function requestFunds(uint256 amount) external onlyProtect {
        //tries toi send from balance
        uint256 availableAssets = IERC20(asset()).balanceOf(address(this));
        uint256 acumulated = availableAssets;
        if (availableAssets < amount) {
            acumulated = availableAssets;
            uint256 len = strategies.length;
            for (uint256 i; i < len; i++) {
                IStrategy currentStrategy = strategies[i].strategy;

                if (address(strategies[i].strategy) == protectStrategy) {
                    continue;
                }

                uint256 pending = amount - acumulated;

                //calculate the max we can get from the strategy
                uint256 avaliableInStrat = currentStrategy.getAvailableAssetsForWithdrawal();

                if (avaliableInStrat == 0) {
                    continue;
                }

                uint256 toWithdraw = pending;
                if (avaliableInStrat < pending) {
                    toWithdraw = avaliableInStrat;
                }

                acumulated += toWithdraw;

                //We control both the length of the array and the external call
                //-next-line unused-return,calls-loop
                currentStrategy.withdraw(toWithdraw, address(this), address(this));
                if (acumulated >= amount) {
                    break;
                }
            }
        }

        //after requesting funds deposits them into the protect strategy
        if (acumulated < amount) {
            revert InsufficientFunds(IStrategy(address(this)), amount, acumulated);
        }
        //-next-line unused-return
        IStrategy(protectStrategy).deposit(amount, address(this));
        emit RequestedFunds(protectStrategy, amount);
    }

Consider addressing the limitation of not being able to update the protectStrategy address, it is recommended to introduce a function that allows for the modification of the protectStrategy address. This function should be restricted to a privileged role, such as the contract owner or an admin, to ensure security and prevent unauthorized changes.

ACKNOWLEDGED: The Concrete team acknowledged this finding.

It has been observed missing zero address checks on the Radiant and Silo constructors:

constructor(
        IERC20 baseAsset_,
        address feeRecipient_,
        address owner_,
        uint256 rewardFee_,
        address siloAsset_,
        address siloRepository_,
        address siloIncentivesController_,
        address[] memory extraRewardAssets,
        uint256[] memory extraRewardFees,
        address vault_
    ) {
        IERC20Metadata metaERC20 = IERC20Metadata(address(baseAsset_));

        siloRepository = ISiloRepository(siloRepository_);
        silo = ISilo(siloRepository.getSilo(siloAsset_));
        if (address(silo) == address(0)) revert InvalidAssetAddress();
        // validate the bridge asset
        if (siloAsset_ != address(baseAsset_)) {
            address[] memory siloAssets_ = silo.getAssets();
            uint256 length = siloAssets_.length;
            uint256 i;
            while (i < length) {
                if (siloAssets_[i] == address(baseAsset_)) {
                    break;
                }
                unchecked {
                    i++;
                }
            }
            if (i == length) revert AssetDivergence();
        }
        siloIncentivesController = ISiloIncentivesController(siloIncentivesController_);
        ISilo.AssetStorage memory assetStorage = silo.assetStorage(address(baseAsset_));
        collateralToken = IERC20(assetStorage.collateralToken);

        // prepare rewardTokens array

        uint256 rewardsLength = extraRewardAssets.length;
        RewardToken[] memory rewardTokenArray = new RewardToken[](rewardsLength + 1);
        // assign the silo reward token first and then process the extra reward tokens
        address[] memory rewards = getRewardTokenAddresses();
        rewardTokenArray[0] = RewardToken(IERC20(rewards[0]), rewardFee_, 0);

        for (uint256 i = 0; i < rewardsLength;) {
            rewardTokenArray[i+1] = RewardToken(IERC20(extraRewardAssets[i]), extraRewardFees[i], 0);
            unchecked {
                i++;
            }
        }

        if (address(collateralToken) == address(0)) revert InvalidAssetAddress();
        __StrategyBase_init(
            baseAsset_,
            string.concat("Concrete Earn SiloV1 ", metaERC20.symbol(), " Strategy"),
            string.concat("ctSlV1-", metaERC20.symbol()),
            feeRecipient_,
            type(uint256).max,
            owner_,
            rewardTokenArray,
            vault_
        );
        //slither-disable-next-line unused-return
        baseAsset_.approve(address(silo), type(uint256).max);
    }

constructor(
        IERC20 baseAsset_,
        address feeRecipient_,
        address owner_,
        uint256 rewardFee_,
        address addressesProvider_,
        address vault_
    ) {
        IERC20Metadata metaERC20 = IERC20Metadata(address(baseAsset_));

        addressesProvider = ILendingPoolAddressesProvider(addressesProvider_);
        lendingPool = ILendingPool(addressesProvider.getLendingPool());
        DataTypes.ReserveData memory reserveData = lendingPool.getReserveData(address(baseAsset_));
        rToken = IAToken(reserveData.aTokenAddress);
        if (rToken.UNDERLYING_ASSET_ADDRESS() != address(baseAsset_)) {
            revert AssetDivergence();
        }
        rewardsEnabled = false;
        incentiveController = IChefIncentivesController(rToken.getIncentivesController());

        __StrategyBase_init(
            baseAsset_,
            string.concat("Concrete Earn RadiantV2 ", metaERC20.symbol(), " Strategy"),
            string.concat("ctRdV2-", metaERC20.symbol()),
            feeRecipient_,
            type(uint256).max,
            owner_,
            _getRewardTokens(rewardFee_),
            vault_
        );
        //slither-disable-next-line unused-return
        baseAsset_.approve(address(lendingPool), type(uint256).max);
    }

Consider adding proper checks on the constructor.

SOLVED: The Concrete team solved the issue by applying recommendations.

The setEnableRewards function allows the owner to enable or disable rewards without any checks. If rewards are enabled when they should not be, this could lead to issues.

/**
     * @dev by setting rewardsEnabled to true the strategy will be able to handle rdnt rewards.
     * check the eligibility criteria before enabling rewards here:
     * https://docs.radiant.capital/radiant/project-info/dlp/eligibility
     */
     function setEnableRewards(bool _rewardsEnabled) external onlyOwner {
        rewardsEnabled = _rewardsEnabled;
        emit SetEnableRewards(msg.sender, _rewardsEnabled);
    }

Consider as the owner verifies eligibility before enabling rewards. Adding a require statement might help to enforce checks:

function setEnableRewards(bool _rewardsEnabled) external onlyOwner {
    // Add any necessary checks here
    require(checkEligibilityCriteria(), "Not eligible for rewards");

    rewardsEnabled = _rewardsEnabled;
    emit SetEnableRewards(msg.sender, _rewardsEnabled);
}

function checkEligibilityCriteria() internal view returns (bool) {
    // Implement the eligibility check logic
}

ACKNOWLEDGED: The Concrete team acknowledged the issue.

Although retireStrategy is restricted to the owner, retiring a strategy is a critical function. Additional checks and safeguards should be considered to prevent misuse.

Centralization Risk Analysis

1. Owner Control:

   • Risk: The owner has significant control over the contract, including the ability to retire the strategy at any time. This centralization means that users of the strategy must trust the owner to act in their best interests.

   • Mitigation: Consider implementing a multi-signature wallet for the owner role to ensure that multiple parties must agree before critical functions like retireStrategy can be executed.

2. Sudden Withdrawal:

   • Risk: The retireStrategy function can withdraw all assets from the strategy, potentially without prior notice to the users. This could disrupt users' investment plans and cause a sudden loss of yield.

   • Mitigation: Implement a time-lock mechanism that delays the execution of the retireStrategy function, providing users with notice and an opportunity to withdraw their funds if they choose to.

To address the centralization risks and enhance the security of the retireStrategy function, consider the following improvements:

1. Time-Lock Mechanism: Introduce a delay between the initiation and execution of the retireStrategy function.

uint256 public retireStrategyTimestamp;
uint256 public constant TIMELOCK_DURATION = 2 days;

function initiateRetireStrategy() external onlyOwner {
    retireStrategyTimestamp = block.timestamp + TIMELOCK_DURATION;
}

function executeRetireStrategy() external onlyOwner {
    require(block.timestamp >= retireStrategyTimestamp, "Timelock not expired");
    _getRewardsToStrategy();
    _protocolWithdraw(rToken.balanceOf(address(this)), 0);
    retireStrategyTimestamp = 0; // Reset the timestamp
}

2. Emergency Withdraw Mechanism: Allow users to withdraw their funds in case of emergency, without waiting for the owner to retire the strategy.

function emergencyWithdraw() external {
    uint256 userBalance = balanceOf(msg.sender);
    _protocolWithdraw(userBalance, 0);
    _transfer(msg.sender, userBalance);
}

3. Notification System: Implement a notification system to alert users when the retireStrategy function is initiated.

ACKNOWLEDGED: The Concrete team acknowledged the issue.

The claim function in _getRewardsToStrategy uses a try-catch block, which is a good practice. However, consider logging the error for better traceability.

function _getRewardsToStrategy() internal override {
        if (!rewardsEnabled) return;
        if (address(incentiveController) == address(0)) return;
        address[] memory _assets = new address[](1);
        _assets[0] = address(rToken);

        //slither-disable-next-line unused-return
        try incentiveController.claim(address(this), _assets) {} catch {}
    }

Consider logging the error for better traceability.

try incentiveController.claim(address(this), _assets) {} catch (bytes memory reason) {
    emit ClaimFailed(reason);
}

event ClaimFailed(bytes reason);

ACKNOWLEDGED: The Concrete team acknowledged the issue.