Security Audit
December 8th, 2022
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
1e814698a0c6ce2d6d6fc3fed6260211086858d4
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
contracts/Bueno721Drop.sol |
|
contracts/BuenoFactory.sol |
|
Click on an issue to jump to it, or scroll down to see them all.
mintBatch()
can fail to enforce BaseSettings.maxPerWallet
mintBatch()
can be used to skirt maxPerTransaction
limits
activatePhase()
are valid phase IDs
setApprovalForAll()
and approve()
Bueno721Drop
unable to update Royalty settings post launch
withdraw()
doesn’t work as intended
mintBatch()
arrays are formatted correctly
getActivePhases()
can be external
hasAllowlist
variable redundant in PhaseSettings
struct
withdraw()
does not need reentrancy guard
We quantify issues in three parts:
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. |
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:
_totalMinted()
in _checkPublicMintConditions()
and airdropPublic()
.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:
baseSetting
's maxSupply
by the difference of a phase’s maxSupply
and amountMinted
.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:
initialize()
to verify the number of tokens for sale match what is intended.mintBatch()
can fail to enforce BaseSettings.maxPerWallet
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:
mintBatch()
.mintBatch()
can be used to skirt maxPerTransaction
limits
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:
mintBatch()
.Note:
maxPerTransaction
removed altogether
activatePhase()
are valid phase IDs
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:
Note: This method was refactored as part of the Q-1 suggestion, and shares a commit SHA
setApprovalForAll()
and approve()
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:
setApprovalForAll()
and approve()
functions.Bueno721Drop
unable to update Royalty settings post launch
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:
withdraw()
doesn’t work as intended
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:
The onlyOwner
modifier isn’t effective as anyone can call release()
for any address and initiate a payment release for a payee.
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.
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:
withdraw()
function and just relying on the PaymentSplitterUpgradeable
’s release()
function. Or,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.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
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:
mintBatch()
to handle under/over payments inside of that function.mintBatch()
arrays are formatted correctly
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:
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:
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,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
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.
getActivePhases()
can be external
The function getActivePhases()
in Bueno721Drop.sol can be marked as external instead of public as there are no internal uses of the function.
Note: Removed as part of 2fd1b5ac8504c642d73c783cb6d2362da5e2883f
hasAllowlist
variable redundant in PhaseSettings
struct
The variable hasAllowlist
could be known from checking if the merkleRoot
variable is equal to zero or not.
withdraw()
does not need reentrancy guard
The function withdraw()
does not need a reentrancy guard because it and the functions it calls all follow the
checks-effects-interaction pattern.
Line 108 and 120 listed below can be removed. The default state of a variable is already equal to zero.
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.