Money Printer - Blueprint Finance

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.

Remediation Plan

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.

Remediation Plan

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.

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

Remediation Plan

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

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

Passing index 4 to the strategies array triggered a panic due to out-of-bounds access.

function test_pullFundsFromSingleStrategy(uint256 amount_) public {
        //uint256 amount_ = 1e18;
        vm.assume(amount_ <= 100_000_000 * 1e18);
        vm.assume(amount_ > 0);
        amount_ = amount_ * 1e18;
        (ConcreteMultiStrategyVault newVault, Strategy[] memory strats) = _createNewVault(false, false, false);
        uint256 hazelsAmount = amount_;
        asset.mint(hazel, hazelsAmount);
        vm.prank(hazel);
        asset.approve(address(newVault), hazelsAmount);

        uint256 preDepositBalance = asset.balanceOf(hazel);
        assertEq(preDepositBalance, amount_, "Predeposit balance == amount");

        vm.prank(hazel);
        uint256 hazelShares = newVault.deposit(hazelsAmount, hazel);

        assertEq(hazelShares, amount_ * 1e9, "Shares with Offset");
        assertEq(
            MockERC4626(address(strats[0].strategy)).balanceOf(address(newVault)),
            amount_.mulDiv(3333, 10_000, Math.Rounding.Ceil) * 1e9,
            "Strategy 0 balance"
        );
        assertEq(
            MockERC4626(address(strats[1].strategy)).balanceOf(address(newVault)),
            amount_.mulDiv(3333, 10_000, Math.Rounding.Ceil) * 1e9,
            "Strategy 1 balance"
        );
        assertEq(
            MockERC4626(address(strats[2].strategy)).balanceOf(address(newVault)),
            amount_.mulDiv(3333, 10_000, Math.Rounding.Ceil) * 1e9,
            "Strategy 2 balance"
        );

        vm.prank(admin);
        newVault.pullFundsFromSingleStrategy(4);

        assertEq(MockERC4626(address(strats[0].strategy)).balanceOf(address(newVault)), 0, "Strategy 0 balance");
        assertEq(
            MockERC4626(address(strats[1].strategy)).balanceOf(address(newVault)),
            amount_.mulDiv(3333, 10_000, Math.Rounding.Ceil) * 1e9,
            "Strategy 1 balance"
        );
        assertEq(
            MockERC4626(address(strats[2].strategy)).balanceOf(address(newVault)),
            amount_.mulDiv(3333, 10_000, Math.Rounding.Ceil) * 1e9,
            "Strategy 2 balance"
        );
    }

Evidence:

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.

Remediation Plan

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.

Remediation Plan

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.

Remediation Plan

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.