Earn (Diff) - Blueprint Finance

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++;
            }
        }
    }

The following Foundry test was used in order to prove the aforementioned issue:

function test_removeVaultAndRedeployDOS() public {
        
        for (uint256 index = 0; index < 10000; index++) { //_deployVault(false)
            ImplementationData memory data =
            ImplementationData({implementationAddress: implementation, initDataRequired: true});

        
            vm.prank(admin);
            vaultManager.registerNewImplementation(keccak256(abi.encode("Test", index)), data);
        
            (bytes memory initData, Strategy[] memory strategies) = _getInitData();
            vm.prank(admin);
            vaultAddress.push(vaultManager.deployNewVault(keccak256(abi.encode("Test", index)), initData));
        }
        /*for (uint256 index = 0; index < 10; index++) {
            address[] memory vaults = vaultRegistry.getAllVaults();
            console.log("VAULT: ",vaults[index]);
        }*/
        //(address vault,) = _deployVault(false);

        vm.prank(admin);
        vaultManager.removeVault(vaultAddress[9999], keccak256(abi.encode("Test", 9999)));

        /*for (uint256 index = 0; index < 9; index++) {
            address[] memory vaults = vaultRegistry.getAllVaults();
            console.log("VAULT: ",vaults[index]);
        }*/

        address[] memory vaults = vaultRegistry.getAllVaults();
        console.log("LEN OLD VAULT: ",vaults.length);
        //assertEq(vaults.length, 0, "Length");
        vm.warp(block.timestamp + 1);
        _deployVault(true);
        address[] memory newVaults = vaultRegistry.getAllVaults();
        console.log("LEN NEW VAULT: ",newVaults.length);
        //assertEq(newVaults.length, 1, "Length");
        
    }

Evidence, test failed consumed a lot of gas:

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

Remediation Plan

SOLVED : The Concrete team solved the issue.

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);
        }
    }

The following Foundry test was used in order to prove the aforementioned issue:

function testFinalizeIssue() public {
        vm.startPrank(owner);
        withdrawalQueue.requestWithdrawal(recipient, 100); //1st request 
        withdrawalQueue.requestWithdrawal(recipient, 200); //2nd request

        withdrawalQueue._finalize(2); //finishing the 2nd request
        //withdrawalQueue._finalize(1);
        withdrawalQueue.prepareWithdrawal(1, 150); //revert RequestNotFoundOrNotFinalized


        uint256 unfinalizedAmount = withdrawalQueue.unfinalizedAmount();
        assertEq(unfinalizedAmount, 200);
        vm.stopPrank();
    }

PoC steps:

1. Two withdrawal requests (request ID 1 and 2.)

2. Finishing the request ID 2.

3. Preparing the withdrawal request ID 1 (n-1). // DoS. It won't be possible to prepare this withdrawal due to a revert error.

4. Even if the vault prepared the request ID 2, after this, it won't be possible to finish the request ID 1 since it InvalidRequestId revert on _finalize function.

Impact: The request ID 1 it won't be possible prepare the withdrawal and claimed it.

Evidence:

Consider ensuring as internal the _finalize function.

Remediation Plan

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.

It was identified that the WithdrawalQueue contract inherited from OpenZeppelin's Ownable library. Ownership of the contracts that are inherited from the Ownable module can be lost, as the ownership is transferred in a single-step process. The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

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);
}

The following Foundry test was used in order to prove the aforementioned issue:

function test_transferOwnership()
        public
    {
      
        (ConcreteMultiStrategyVault newVault,) = _createNewVault(true, false, true);

        WithdrawalQueue queue = new WithdrawalQueue(address(newVault));
        vm.prank(address(newVault));
        queue.transferOwnership(hazel);
       
    }

It was possible to transfer the ownership. Evidence (test passed):

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

Remediation Plan

RISK ACCEPTED: The Concrete team accepted the risk of 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.

Remediation Plan

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_);
}

Remediation Plan

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.

Remediation Plan

ACKNOWLEDGED: The Concrete team acknowledged this finding.