<div align="center">
<center>
<img src="https://avatars.githubusercontent.com/u/167952721" height="350" alt="@offbeatsecurity" style="margin-bottom: 20px;">
<h1>Blaze Contracts Review</h1>
<h3>July 6, 2024</h3>
<p>Prepared for Blaze</p>
<p>Conducted by: </p>
<p>Kurt Willis (phaze)</p>
<p>Richie Humphrey (devtooligan)</p>
</center>
</div>

## About the **Blaze Contracts Review**

From the docs:

> Blaze is the creator platform that lets you create and mint NFTs with novel mechanisms.
>
> Blaze offers a suite of features that caters to all demographics of NFT enthusiasts, including creators, traders, and collectors.

## About **Offbeat Security**

Offbeat Security is a boutique security company providing custom solutions for complex and novel crypto projects. Our mission is to elevate the blockchain security landscape through invention and collaboration.

## Scope
The [src]() folder of the `blaze-core` repo was reviewed at commit 2539728592a4545dd873cbdf195c2327c253ea0c.

The following **7 contracts** were in scope:
- BlazeTypeBase.sol
- BlazeType.sol
- BlazeTypeRefundable.sol
- BlazeManager.sol
- The files Constants.sol, Structs.sol, and Errors.sol are inherited by these above files and also in scope.
- The Blast interfaces, src/vendor/, and src/deprecated along with scripts/ are not in scope

## Summary of Findings

| Identifier | Title | Severity | Fixed |
| ---------- | ---------------------------------------------------------------------------------------- | ------------- | ----- |
| [\[C-01\]](#C-01-Blaze-front-end-does-not-employ-validation-checks-for-deployed-contracts) | Blaze front-end does not employ validation checks for deployed contracts | Critical | |
| [\[H-01\]](#H-01-Rescue-functions-allow-owner-to-steal-refundable-amounts) | Rescue functions allow owner to steal refundable amounts | High | |
| [\[H-02\]](#H-02-Mint-is-non-functioning-for-NFT-contracts-using-ERC-20-tokens) | Mint is non functioning for NFT contracts using ERC-20 tokens | High | |
| [\[H-03\]](#H-03-NFT-contracts-that-use-whitelist-phases-cannot-be-deployed) | NFT contracts that use whitelist phases cannot be deployed | High | |
| [\[L-01\]](#L-01-Maximum-mint-per-call-limits-dont-affect-smart-contracts) | Maximum mint per call limits don't affect smart contracts | Low | |
| [\[L-02\]](#L-02-Mint-can-be-overpaid-in-ETH-by-users) | Mint can be overpaid in ETH by users | Low | |
| [\[L-03\]](#L-03-Incorrect-revert-logic-in-ownerBatch-function) | Incorrect revert logic in `ownerBatch` function | Low | |
| [\[L-04\]](#L-04-The-invariant-of-non-overlapping-whitelist-phases-can-be-broken) | The invariant of non-overlapping whitelist phases can be broken | Low | |
| [\[L-05\]](#L-05-The-deployBlazeType-function-is-currently-not-used-by-the-system) | The deployBlazeType function is currently not used by the system | Low | |
| [\[I-01\]](#I-01-Storage-variables-for-refunds-are-unused-and-may-be-removed) | Storage variables for refunds are unused and may be removed | Informational | |
| [\[I-02\]](#I-02-Yield-on-NFT-contracts-USDB-balances-is-earned-and-accumulated-but-can-never-be-claimed) | Yield on NFT contracts' USDB balances is earned and accumulated but can never be claimed | Informational | |
| [\[I-03\]](#I-03-Incorrect-reinitializer-version-set-for-BlazeManager) | Incorrect `reinitializer` version set for `BlazeManager` | Informational | |
| [\[I-04\]](#I-04-Creator-could-bypass-minting-fee-with-owner-mints) | Creator could bypass minting fee with owner mints | Informational | |

## Centralization risks
- When the `PROTOCOL_RECEIVER` is set to a malicious contract, it can block all mints by reverting when fees are transferred.

- The `owner` of the `BlazeManager` contract can set high fees for profit or to block minting.

- The Blast points operator inside the `BlazeType` contracts is configured to be a hardcoded address owned by the Blaze team

- The BlazeManager is upgradeable and may be upgraded at any time by the Blaze team.

## Protocol design recommendations

- **Simplify fee structure:** In the case when a token currency is used, it might be cumbersome for the users to require providing two different currencies (native and a token) for the minting. Consider taking a protocol fee as a percentage of whichever currency is being used. Or consider whether the yield generated by the contracts might be enough revenue for Blaze.

- **Implement factory and clone pattern:** Consider implementing a factory and clone pattern. The `BlazeManager` currently allows creating `BlazeType` contracts, however this is not a must. They can also create a contract by simply deploying their own `BlazeType` contract via the frontend UI. As Blaze has to hope that the deployed contract will be the exact contract specified, this allows for some degree of control for the user. The risk for potential future integration issues and attack can be minimized by employing a central factory that acts as a registry by either directly storing addresses or emitting events for trusted contracts deployed by the factory.

- **Enable direct NFT contract interaction:** Consider allowing the users to directly call the NFT contracts in order to mint tokens instead of passing through the `BlazeManager`. The current fee system can still be hardcoded into the implementation contracts.

- **Adopt a "pull" pattern for payments:** Consider adhering to a "pull" instead of a "push" pattern. Currently the `BlazeManager` calls out to the protocol fee receiver for every mint and the refund mechanism of `BlazeTypeRefundable` calls out to the owner for each call. Instead consider tracking the owed balances locally first and Allowing the owner of the respective contracts to claim the balances at once.

- **Implement signature-based whitelisted mints:** While definitely not a must, consider allowing whitelisted mints to be performed via signatures. This can be more efficient and gives NFT creators additional flexibility. The tradeoffs are discussed in some articles (e.g. https://x.com/Jeyffre/status/1807008534477058435), however some nuances in gas costs apply to layer 2 blockchains.

- **Properly handle edge cases:** In `BlazeType.withdraw()` if an amount is passed which is greater than the `_totalFundsReceived` and also greater than the contract balance, then the transaction will revert. Further, when minting close to the max supply, if the desired mint amount exceeds the max supply, then the function will revert. Instead, consider allowing the user to mint the leftover amount and refund the rest for a better user experience.

## Additional Recommendations

- **Remove dead code:** Twice in the `BlazeManager` contract, the success of a call is checked in a require statement and then on the next line there is an if statement that checks if the call did not succeed. However, this code will never be reached if the call was not sucessful since it reverts on the prior line.

- **Avoid magic constants:** "Magic values" are used throughout the codebase such as the hardcoded hex value of function selectors or the integer value of the number of seconds in a day. Consider using constants or built in Soldity functions sunce as `1 days` instead of magic values for improved readability.

- **Adhere to the CEI pattern:** The Checks-Effects-Interactions (CEI) pattern should be used consistently throughout the code base. While we did not identify any re-entrancy issues in the current code, the extensive interactions with ETH increase the likelihood of a reentrancy vulnerability being introduced in the future which make adhering to this best practice even more important.

- **Allow users to opt out of automatic yield generation:** The contract hardcodes the enabling of yield from Blast. However, for some use cases in some jurisdictions, this may present a legal challenge as it may cause the NFT to fall under the definition of a security. As such, consider allowing NFT creators the option to opt out of automatically enabling yield.

- **Clean up unused variables and imports throughout the codebase.**

## Detailed Findings

## Critical Findings

## [C-01] Blaze front-end does not employ validation checks for deployed contracts

The Blaze protocol smart contracts by design do not have a central registry where `BlazeType` contracts are stored. As it is not required to deploy `BlazeType` mint contracts through the `BlazeManager`, the front-end sets up a deploy transaction and has to trust the user to execute the bytecode as intended.

This means that users can deploy modified `BlazeType` contracts which are then registered and listed on the https://blaze.ong website and/or possibly modify database entries. A few possible attack scenarios are:

- Users can swap out `manager` contract address in `BlazeType` contract
- Users can deploy modified `BlazeTypeRefundable` contract with backdoor to have users believe tokens are refundable and steal all ETH
- Users can opt not to deploy a contract, but simply return their EoA address which is entered into the backend database. Calling the mint function on the front-end would lead to immediate token transfers to the designated EoA
- Users can deploy an invalid (reverting) contract possibly breaking the https://blaze.ong/explore page
- Database is unprotected and users can modify or corrupt database

This exploit scenario is exemplified by swapping out the manager contract in the Blaze front-end.

When arriving at https://www.blaze.ong/create, the user is prompted to fill in their collection details. Once complete, the user is able to click the "DEPLOY" button which prepares the MetaMask transaction.


If we switch over to the 'HEX' tab we can see the data that is being deployed.


This data is the contract deployment bytecode with the appended constructo arguments (including the manager address).

      const hash = await walletClient.deployContract({
        abi,
        account: address,
        args: [params, formData.collectionName, formData.symbol, managerAddress, formData.ownerAddress as Address],
        bytecode,
      })

By using Chrome's devtools, we can locate and modify this manager address and swap it out for our own.


When the attacker clicks on "DEPLOY" again, we see the modified bytecode containing the spoofed deployer address.


### Recommendation

Mission critical javascript code server should be executed server side instead of client side. In order to safely ensure that the deployed contract equals the trusted `BlazeType` contract, it should be deployed from a central registry contract or validated independently.

The backend system should work independently and fault free from user interactions. Users should be given the least amount of privileges. This would involve the backend independently creating the database from the on-chain data, or perhaps employing additional safety measures from user provided data.

## High Findings

## [H-01] Rescue functions allow owner to steal refundable amounts

An owner of an refundable NFT can withdraw all value from the contract using a rescue function and thereby remove the ability of the holders to collect refunds.

The purpose of the rescue functions `rescueETH()` and `rescueERC20()` is to retrieve tokens mistakenly sent to the contract. However, an owner could also use them to steal all the tokens from a refundable NFT contract.

### Recommendation
Override the rescue methods allow the owner to only remove any excess token value. For example:

function rescue(address _token, address _to) external onlyOwner {
    if (_token == address(0)) {
        uint256 amount = address(this).balance;

        if (_token == btStorage.tokenCurrency) amount -= _totalFundsReceived;

        (bool success,) = _to.call{ value: amount }("");
        require(success, "Native transfer failed");
    } else {
        uint256 amount = IERC20(_token).balanceOf(address(this));

        if (_token == btStorage.tokenCurrency) amount -= _totalFundsReceived;

        IERC20(_token).transfer(_to, amount);
    }
}

## [H-02] Mint is non functioning for NFT contracts using ERC-20 tokens

The `mint()` function will always revert when the NFT contract uses an ERC-20 token as its token currency.

When the `_handleMint()` is called from `mint()` or `whitelistMint()`, it will attempt a `transferFrom` on the token and pass the `msg.sender` as the `from` value.

    if (btStorage.tokenCurrency == address(0)) {
      if (msg.value < totalPrice) revert InsufficientFunds();
    } else {
      IERC20 token = IERC20(btStorage.tokenCurrency);
      token.transferFrom(msg.sender, address(this), totalPrice);
    }

However, the `msg.sender` will always be the `BlazeManager` contract and not the user calling `mint` since, on the `BlazeTypeBase` contract, both mint functions are privileged and only callable by the manager:

  function _preliminaryMintChecks(uint256 _amount) internal view {
    if (msg.sender != manager) revert Unauthorized();
    if (_nextTokenId() + _amount > btStorage.maxSupply) revert MaxSupplyReached();
  }

There is no logic in the `BlazeManager` contract to handle the required tokens, and even if the tokens were optimistically transferred to the manager first, the transaction would still revert due to inadequate allowance.

### Recommendation
Consider adapting the manager to pass in the calling user's address to the `mint` function. The address passed to the `BlazeType` must not be user-controlled.

Alternatively, the tokens can be transferred into the manager contract first and then on to the `BlazeType` contract. This, however, requires the manager contract to approve the `BlazeType` contract for token spending allowances. This can be done in the manager's `deployBlazeType` function.

## [H-03] NFT contracts that use whitelist phases cannot be deployed

Deploying an NFT contract with one or more whitelist phases will always revert.

Upon deployment, the `_validateWhitelistPhases` is called from the constructor.

  function _validateWhitelistPhases(WhitelistPhase[] memory _phases) internal view {
    // Check for length > 3
    if (_phases.length > 3) revert InvalidWhitelistPhases();

    // Check that phase start must be before phase end, phase ends must be before public
    // mint start, and that phases do not overlap.
    uint256 prevEnd;
    for (uint256 i; i < _phases.length;) {
      if (
        _phases[i].start > _phases[i].end || _phases[i].end > btStorage.start
          || (_phases[i].start != 0 && _phases[i].start < prevEnd)
          || (_phases[i].start == 0 && _phases[i].end != 0)
      ) {
        revert InvalidWhitelistPhases();
      }
      prevEnd = _phases[i].end;
      unchecked {
        ++i;
      }
    }

At the time this function is called, `btStorage` has not been set yet so all storage values, including `btStorage.start` will be zero by default. This means that the second conditional in the if statement, `_phases[i].end > btStorage.start`, will always result in `true` for any non-zero phases which will cause the function to revert.

### Recommendation
Set the public mint start time in storage prior to calling this function.

## Low Findings

## [L-01] Maximum mint per call limits don't affect smart contracts

The `maxMintsPerCall` setting is intended to limit the number of tokens that can be minted in a single call. While this technically works, a smart contract can combine multiple calls in one transaction by using `multicall` and effectively bypass the limit.

### Recommendation

A common way to mitigate this proplem is to only allow EoAs (externally owned accounts) to interact with the entrypoint smart contract (in this case the `BlazeManager` contract) for minting. This solution would involve adding a requirement that `tx.origin == msg.sender` to the `BlazeManager` contract. The downside with this solution is that it would disallow smart contract wallets to mint tokens—a currently rather rare occurence, as every minting contract might have a different signature requiring a manual setup. Checking that the caller does not contain any code `msg.sender.code.length != 0` is ineffective as contract's do not contain any code during contract initialization in their constructor.

With the possible future inclusion of EIP-7702 (favored over EIP-3074's `AUTH` and `AUTHCALL` opcodes), the `tx.origin == msg.sender` check might not be effective anymore as EIP-7702 allows setting an EoA's contract code temporarily for the duration of the transaction execution. If this EIP was included, the procedure could be to release a new version of the `BlazeManager` contract which now employs both checks: `tx.origin == msg.sender && msg.sender.code.length == 0`.

A more involved alternative solution could make use of transient storage to track when a call to `mint` has been already made during the execution of the transaction. While this solution might be effective, it might not be compatible with many chains as `tstore` and `tload` are relatively new opcodes.

## [L-02] Mint can be overpaid in ETH by users

The `BlazeManager.mintBlazeType` is payable to allow for the payment of fees and it also passes the remaining msg.value on to the NFT contract to cover the minting costs for those NFT contracts that have chosen ETH as their currency token. However, the logic in the mint functions in `BlazeType` is missing two checks:
1) For contracts that deal in an ERC-20 contract, there is no check disallowing `msg.value > 0`
2) For contracts that deal in ETH, there is no check disallowing `msg.value > amount`

This means that a user may over pay in ETH when calling `mintBlazeType` and they will lose access to their funds.

### Recommendation
Consider adding the missing checks mentioned above.

## [L-03] Incorrect revert logic in `ownerBatch` function

The `ownerBatch` function contains logic which intends to bubble up the return data in the case of an unsuccessful delegatecall:

      (bool success, bytes memory result) = address(this).delegatecall(_calls[i]);
      if (!success) {
        if (result.length < 68) revert("Transaction reverted without a reason");

        bytes memory returnData;

        assembly {
          returnData := add(returnData, 0x04)
        }

        revert(string(returnData));

However, there are mistakes in this logic. First of all, within the assembly block, the `add` opcode is applied to the newly initialized `returnData` variable (pointing to the zero slot) instead of the `result` variable. Secondly, the method used to extract the error message makes the assumption that the error is always encoded as `Panic(string)`. As this only works for a subset of errors and might not always work or return an illegible string, a more general approach is recommended.

### Recommendation
Consider replacing this logic with a call to `handleReturnData()` as is done in other parts of the code.

## [L-04] The invariant of non-overlapping whitelist phases can be broken

Upon deployment, the `_validateWhitelistPhases` is called in the constructor with the selected whitelist phases.

  function _validateWhitelistPhases(WhitelistPhase[] memory _phases) internal view {
    // Check for length > 3
    if (_phases.length > 3) revert InvalidWhitelistPhases();

    // Check that phase start must be before phase end, phase ends must be before public
    // mint start, and that phases do not overlap.
    uint256 prevEnd;
    for (uint256 i; i < _phases.length;) {
      if (
        _phases[i].start > _phases[i].end || _phases[i].end > btStorage.start
          || (_phases[i].start != 0 && _phases[i].start < prevEnd)
          || (_phases[i].start == 0 && _phases[i].end != 0)
      ) {
        revert InvalidWhitelistPhases();
      }
      prevEnd = _phases[i].end;
      unchecked {
        ++i;
      }
    }

These must satisfy certain conditions for them to be coherent.
> Check that phase start must be before phase end, phase ends must be before public mint start, and that phases do not overlap.

The invariant that is trying to be enforced is that all whitelist phases are given in a non-overlapping sequential ordering before the public mint starts. This invariant can be violated in a few ways:

Phases that have a start and end time of `(0, 0)` can always be added. This means that a phase list of `[(100, 150), (0, 0), (50, 200)]` is a valid sequence. This list is neither sequential nor non-overlapping.

Even without the special case of `(0, 0)`, there is some ambiguity with regards to what is considered non-overlapping. The sequence `[(100, 200), (200, 300)]` shares a common point at `t = 200`.

When calling the `whitelistMint` function, the phase is given as a parameter.

    function whitelistMint(
        uint8 _phase,
        bytes32[] calldata _merkleProof,
        address _to,
        uint256 _amount
    ) external payable override {
        _preliminaryMintChecks(_amount);

        // Refundable logic
        WhitelistPhase memory thisPhase = btStorage.whitelistPhases[_phase];

Given the previous phase sequence of `[(100, 200), (200, 300)]`, at `block.timestamp = 200` a whitelist mint can be executed for both phases `0` and `1` at the same time. This is because the phase validation includes the start and end time.

        // Mint validation
        WhitelistPhase memory thisPhase = btStorage.whitelistPhases[_phase];
        if (thisPhase.start > block.timestamp || thisPhase.end < block.timestamp) {
            revert WhitelistPhaseNotActive();
        }

### Recommendation
Change the mint validation to not include the phase end time.

        // Mint validation
        WhitelistPhase memory thisPhase = btStorage.whitelistPhases[_phase];
-       if (thisPhase.start > block.timestamp || thisPhase.end < block.timestamp) {
+       if (thisPhase.start > block.timestamp || thisPhase.end <= block.timestamp) {
            revert WhitelistPhaseNotActive();
        }

As well as the initial `_validateWhitelistPhases()` called from the constructor:

     for (uint256 i; i < _phases.length;) {
       if (
         _phases[i].start > _phases[i].end || _phases[i].end > btStorage.start
-          || (_phases[i].start != 0 && _phases[i].start < prevEnd)
+          || (_phases[i].start != 0 && _phases[i].start <= prevEnd)
           || (_phases[i].start == 0 && _phases[i].end != 0)
       ) {
         revert InvalidWhitelistPhases();

## [L-05] The deployBlazeType function is currently not used by the system

The current Blaze application relies on a user interacting with the front end to create an NFT collection which is then registered in the database and displayed in the gallery on the website.

There is currently a `deployBlazeType` function which can deploy a new collection similar to how the front end does, however it does not register the collection in the database and it will not get displayed in the gallery on the website. This function is not used at all by the protocol and is not necessary.

A user or integrator may call this function unknowingly with the expectation that it will be included on the website. This mistake would cost the user the deployment costs and could hurt the protocol's reputation.

### Recommendation

Remove this function.

## Informational Findings

## [I-01] Storage variables for refunds are unused and may be removed

In `BlazeTypeRefundable`, the storage slot `btRefundableStorage.` is never set so it's value will always be zero.

It is used in one check in the `refund()` function:

function refund(uint256 _tokenId) external {
    if (_tokenId < btRefundableStorage.mintsToOwner) revert NonRefundable();
  ...

Because the `btRefundableStorage.mintsToOwner` is never set and defaults to zero, this if statement will always evaluate to `false` and it will never revert. However, during the owner minting process, `RefundData` is never generated. This means that if `refund()` is called on any of the owner NFT's, it will pass this check but the amount transferred will be zero since the `refundAmount` is never set.

This means that the check is not needed and since `btRefundableStorage.mintsToOwner` is not used anywhere else, it may be removed.

Alternatively, the `RefundData.refundAmount` could be removed as the refunded amount always equals `btStorage.mintPrice` and doesn't need to be tracked per user. This still require adequately tracking which NFTs were minted by which user.

In its current version, `RefundData.refunded` is only being set, but it is never checked.

### Recommendation
Remove all references to `btRefundableStorage.mintsToOwner` and `RefundData.refunded`.

A simpler design could involve tracking only the unlock time in a single mapping `mapping(uint256 tokenId => uint256) unlockTime`, as the price can read from `btStorage.mintPrice`. Whether the `unlockTime` for a given NFT is non-zero can be used to determine whether a NFT has been refunded already or minted by the owner.

## [I-02] Yield on NFT contracts' USDB balances is earned and accumulated but cannot be claimed

By default, all addresses on Blast that hold USDB will earn yield. Unlike ETH, this feature is enabled by default and does not need to be turned on with a call to the Blast contract.

However, the `BlazeType` contracts do not contain a function to claim the rewards since there is no other way to claim them.

*Note regarding severity*: The project team stated they are aware of this issue and have chosen not to allow users to claim the yield. Since this was an intentional design decision, the severity has been reduced to informational.

### Recommendation
Add a `claim()` function to the `BlazeType` contract to allow owners to claim the yield they have earned.

## [I-03] Incorrect `reinitializer` version set for `BlazeManager`

The `BlazeManager` is an upgradeable UUPS proxy contract. As such it exhibits an `initialize` function which is protected by the `reinitializer` modifier which is found in the OpenZeppelin `UUPSUpgradeable` contract. The `reinitializer` modifier allows specifying an implementation version. With this modifier, the `initialize` function can only be called once per implementation version.

    function initialize() external reinitializer(2) {
        __UUPSUpgradeable_init();
        __Ownable_init(msg.sender);
        IBlast(0x4300000000000000000000000000000000000002).configureClaimableGas();
        IBlast(0x4300000000000000000000000000000000000002).configureGovernor(
            PROTOCOL_RECEIVER
        );
    }

The currently deployed on-chain `BlazeManager` implementation contract (https://blastscan.io/address/0x47e6a9f50ab4cba098a2cb38258e951b9001a378#code) features the exact same version number `reinitializer(2)` as the given implementation contract. As such it cannot be executed again.

### Recommendation

As the current `initialize` function is likely not intended to be called again (as the contract's gas claiming configuration cannot be set after the governor has already been set), it should be removed from the contract.

## [I-04] Creator could bypass minting fee with owner mints

When creating a new collection, a creator could mint the entire supply as owner mint. This would bypass the minting fee since fees are not charged on owner mints. They could then sell the NFT's on an NFT marketplace.

### Recommendation

Consider charging a fee for owner mints over a certain threshold.