Bueno.art A-1

Security Audit

December 8th, 2022

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Bueno.art's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from November 28, 2022 to December 2, 2022.

The purpose of this audit is to review the source code of certain Bueno.art Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 2 - - 2
Medium 3 - - 3
Low 6 - - 6
Code Quality 5 - - 5
Gas Optimization 1 - - 1

Bueno.art was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Contract SHA256
contracts/Bueno721Drop.sol

3bdafa453088eef1188fcfce64d7a887eff3251fdcab1a0a48d01b7c3aed5ed8

contracts/BuenoFactory.sol

5c9d62b41495435ad582673874a45911cf18e6a5dcff39f9222d17d28ebe7daf

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Public Mint can revert when total project mints exceed the Public Mint limit

Topic
Use Cases
Status
Fixed (closed source)
Impact
High
Likelihood
High

In Bueno721Drop.sol’s _checkPublicMintConditions() and airdropPublic(), the wrong accounting variable is used to gauge if enough tokens are available for a public mint. This will result in public mints which should succeed to fail. The below condition on line 557, and a similar condition on line 302, is used to check if the public mint has enough tokens left to satisfy the user request:

if (_totalMinted() + quantity > baseSettings.maxSupply) {
    revert SoldOut();
}

The checks use _totalMinted(), which tracks all mints which have occurred included phase mints, instead of just the amount of public mints which have happened so far. The needed variable of total public mints so far is not actually tracked in the code anywhere.

The current code will revert when the amount of mints which have occurred globally in the project exceeds the total amount of mints available to the public mints.

Remediations to Consider:

  • Keep track of the number of public mints and use that as the comparison instead of _totalMinted() in _checkPublicMintConditions() and airdropPublic().
H-2

Ending a phase will increase the Public Mint limit by too much

Topic
Spec
Status
Fixed (closed source)
Impact
High
Likelihood
High

The spec specifics that the remaining supply of a phase should be transferred to the mintPublic roll-over phase. Currently, instead of the remaining phase supply being transferred, an ended phase’s whole initial supply is transferred in Bueno721Drop.sol’s endPhase():

/**
* @dev Permanently closes a phase by capping the supply & releasing it
*/
function endPhase(uint256 phaseIndex) public onlyOwner {
   // if the phase never had supply, there is nothing to do
   if (saleState.phases[phaseIndex].maxSupply == 0) {
       revert InvalidPhase();
   }

   // transfer the supply to the global supply
   baseSettings.maxSupply += saleState.phases[phaseIndex].maxSupply;

   // remove the supply from the phase
   saleState.phases[phaseIndex].maxSupply = 0;
}

This is problematic because during a phase’s mint functions this maxSupply field is not decreased. This results in the mintPublic's supply being increased by too much, possibly creating tokens that have no associated artwork.

Remediations to consider:

  • Only increasing the baseSetting's maxSupply by the difference of a phase’s maxSupply and amountMinted.
M-1

No validation on project’s total initial token supply

Topic
Input Validation
Status
Fixed (closed source)
Impact
Medium
Likelihood
Medium

In Bueno721Drop.sol’s initialize() function, no check is made to ensure that the total amount of available tokens matches any specified number. Considering the importance of only selling a specific number of tokens which have associated art, combined with the complexity of setting up the initial phases and public mint token limits, we’d recommend adding a check in initialize() to ensure that the intended number of tokens are actually put up for sale.

Remediations to consider:

  • Adding a check in initialize() to verify the number of tokens for sale match what is intended.
M-2

mintBatch() can fail to enforce BaseSettings.maxPerWallet

Topic
Spec
Status
Fixed (closed source)
Impact
Medium
Likelihood
Medium

In Bueno721Drop.sol’s mintBatch() function, users can mint more tokens than what is specified in the project’s BaseSettings.maxPerWallet. This is because the check for the limit is made using the total number of minted tokens per user in the mint checking functions on lines 482, 531, and 561:

if (
    baseSettings.maxPerWallet > 0 &&
    _numberMinted(wallet) + quantity > baseSettings.maxPerWallet
) {
    revert ExceedMaxPerWallet();
}

This is problematic because mintBatch() only mints the tokens for the phases once at the end of the function, so these checks will fail to account for the total number of tokens a user has minted in the earlier phases processed by the batch. For example, assume that the BaseSettings.maxPerWallet is 5 and user Alice uses mintBatch() to mint 4 tokens in two different phases. Alice will have successfully minted 8 tokens when the global max was 5 which is against spec.

Remediations to consider:

  • Minting tokens per phase processed. Or,
  • Checking for the global max once at the end of mintBatch().
M-3

mintBatch() can be used to skirt maxPerTransaction limits

Topic
Spec
Status
Fixed (closed source)
Impact
Medium
Likelihood
Medium

In Bueno721Drop.sol, users can use the mintBatch() function to work around the PhaseSettings.maxPerTransaction limit by buying from the same phase multiple time during the transaction call. This is spec breaking.

Remediations to consider:

  • Not allowing users to purchase from the same phase multiple times in mintBatch().
Response by Bueno.art

Note: maxPerTransaction removed altogether

L-1

Missing validation that specified IDs in activatePhase() are valid phase IDs

Topic
Input Validation
Status
Fixed (closed source)
Impact
Low
Likelihood
Low

In Bueno721Drop.sol’s activatePhase() function, there is no guarantee that the activated phases in the phaseIndices array are valid phases. This violates the principal that the internal state of the contract is consistent.

Remediations to consider:

  • Checking that the activated phases exist.
Response by Bueno.art

Note: This method was refactored as part of the Q-1 suggestion, and shares a commit SHA

L-2

Missing suggested OperatorFilterRegistry's function coverage on setApprovalForAll() and approve()

Topic
Interoperability
Status
Fixed (closed source)
Impact
Low
Likelihood
High

It is suggest that use of OpenSea’s OperatorFilterRegistry includes coverage of the setApprovalForAll() and approve() functions. This is to ensure that tokens cannot be listed on platforms which they will not be able to be sold on. See OpenSea’s example contract for implementation guidance.

Remediations to consider:

  • Adding the onlyAllowedOperatorApproval() modifier to Bueno721Drop.sol’s setApprovalForAll() and approve() functions.
L-3

Owner of Bueno721Drop unable to update Royalty settings post launch

Topic
Protocol Design
Status
Fixed (closed source)
Impact
Medium
Likelihood
Medium

Currently there is no way for the owner of a Bueno721Drop to update the Royalty info for their project. As market conditions and community preferences can change over time, not being able to update Royalty settings can lead to decreased project success.

Remediations to consider:

  • Adding the ability for owners to change their project’s Royalty settings.
L-4

withdraw() doesn’t work as intended

Topic
Use Cases
Status
Fixed (closed source)
Impact
Low
Likelihood
Medium

In Bueno721Drop.sol, the withdraw() function has non-intended behavior due to the possibility of malicious actors and the existence of the public release() function inherited from PaymentSplitterUpgradeable.sol. The function can fail to work as intended in the following ways:

  1. The onlyOwner modifier isn’t effective as anyone can call release() for any address and initiate a payment release for a payee.

  2. If someone does utilize the release() function, subsequent calls made to withdraw() before the contract’s next money-gaining event will fail. This is due to release() reverting when an account has no money to claim.

  3. If there is a malicious user who reverts on money transfers to themselves, then they can prevent the withdraw() function from ever finishing an execution.

Remediations to consider:

  • Removing the withdraw() function and just relying on the PaymentSplitterUpgradeable’s release() function. Or,
  • Restrict access to PaymentSplitterUpgradeable’s release() function to only the owner and allowing the owner to specify which addresses to pay out in withdraw(). Note: this remediation might be giving too much power to the owner of the contract.
Response by Bueno.art

Note: Updated to prevent reverting in case of 0 releasable funds for a payee. We are not worried about the bad actor case, as this is a convenience method and PaymentSplitter.release() can still be individually called

L-5

Users can overpay

Topic
Use Cases
Status
Fixed (closed source)
Impact
Medium
Likelihood
Medium

In Bueno721Drop.sol, users are able to pay more than needed for their NFT purchases. This is due to the price checks being written as a greater-than instead of an equal-to in the condition checking functions:

if (balance < quantity * baseSettings.price) {
    revert InvalidPrice();
}

This can lead to poor user experience if users overpay.

Remediations to consider:

  • Changing the checking functions to check for equal-to conditions and restructure mintBatch() to handle under/over payments inside of that function.
L-6

No check that mintBatch() arrays are formatted correctly

Topic
Input Validation
Status
Fixed (closed source)
Impact
Low
Likelihood
Low

In Bueno721Drop.sol, no check is made to ensure that the argument arrays are of the same size. This can lead to array-out-of bound errors which aren’t user friendly to debug.

Remediations to consider:

  • Checking that all input arrays are of the same size.
Q-1

Keeping track of active phases with an array is an anti-pattern

Topic
Code Quality
Status
Fixed (closed source)
Quality Impact
Medium

Note: this is a code quality suggestion and not a security suggestion. The code changes suggested here are slightly larger than the changes suggested in other issues found so far. We will leave it up to your teams’s discretion on time vs. code quality tradeoff on whether to implement this suggestion.

Bueno721Drop.sol currently uses an array to keep track of the active phases. This isn’t necessarily problematic from a security perspective but it is a Solidity anti-pattern.

Using an array to keep track of active phases isn’t recommended because as more phases are active, more gas is spent verifying and updating the active phases.

For example, in the activatePhase() function, if you have 100 active phases and you want to deactivate a single phase, you have to re-write storage slots the remaining 99 phases (which is gas intensive):

/**
* @dev Specify which phases are active. By omitting phase indices, you are effectively closing those phases
* Public sale can be simultaneously activated via `_isPublicActive` argument.
*/
function activatePhase(uint64[] calldata phaseIndices, bool _isPublicActive)
   external
   onlyOwner
{
   saleState.activePhaseIds = phaseIndices;

   if (_isPublicActive != isPublicActive) {
       isPublicActive = _isPublicActive;
   }
}

Additionally, searching to see if a phase is active will grow in gas intensity with the size of the list:

function isPhaseActive(uint256 phaseIndex) internal view returns (bool) {
   uint256 activePhaseLength = saleState.activePhaseIds.length;
   for (uint256 i = 0; i < activePhaseLength; ) {
       if (saleState.activePhaseIds[i] == phaseIndex) return true;

       // activePhaseLength has a maximum value of 2^256 - 1
       unchecked {
           ++i;
       }
   }

   return false;
}

Best practice solidity coding typically includes keeping the gas cost of running a section of code the same at any scale. This is because (1) users have to pay gas costs directly and (2) there is a hard limit to the amount of gas a transaction can take, capped currently at 30 million gas units per block. This can lead to non-intuitive functioning of array-based code.

For example, the code suggests that the number of phases is capped at 2**64, but it is actually capped at the amount of gas which is consumed by the initializer() function. Considering that a single storage slot write costs 20_000 per non-zero write*, a user would consume the 30 million block gas limit setting a single storage slots for 1500 phases.

Due to the unique runtime environment of the EVM you’ll find developers utilizing map data structures with additional helper tracking variables instead of using arrays.

Remediations to consider:

  • Using a map data structure to keep track of if a phase is active or not. This can be achieved by replacing the uint64[] activePhaseIds with a mapping(uint256 => bool) paired with metadata about the max phase ID. This will remove the loop from isPhaseActive() and the code in activatePhase() can be modified in a variety of ways to still allow for multiple phases to be toggled at once. Or,
  • Adding a bool into the PhaseSettings struct indicating if a phase is active or not instead of the uint64[] activePhaseIds structure. Additionally make other similar changes listed above.

* See gas costs for opcodes on page 27 of the EVM yellow paper

Q-2

No events are emitted to track state changes

Topic
Code Quality
Status
Fixed (closed source)
Quality Impact
Medium

Currently no events are emitted to track state changes in Bueno721Drop.sol. It is a good practice to include events in contracts in general. Without them there is no easy way to track the history of the projects. In addition, they're useful for front end applications interacting with contracts. We’d recommend adding events to all functions which change any state internally in the contract.

Q-3

getActivePhases() can be external

Topic
Code Quality
Status
Fixed (closed source)
Quality Impact
Low

The function getActivePhases() in Bueno721Drop.sol can be marked as external instead of public as there are no internal uses of the function.

Response by Bueno.art

Note: Removed as part of 2fd1b5ac8504c642d73c783cb6d2362da5e2883f

Q-4

hasAllowlist variable redundant in PhaseSettings struct

Topic
Code Quality
Status
Fixed (closed source)
Quality Impact
Low

The variable hasAllowlist could be known from checking if the merkleRoot variable is equal to zero or not.

Q-5

withdraw() does not need reentrancy guard

Topic
Code Quality
Status
Fixed (closed source)
Quality Impact
Low

The function withdraw() does not need a reentrancy guard because it and the functions it calls all follow the checks-effects-interaction pattern.

G-1

Initializing variables to default value wastes gas

Topic
Gas Optimization
Status
Fixed (closed source)
Gas Savings
Low

Line 108 and 120 listed below can be removed. The default state of a variable is already equal to zero.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Bueno.art team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.