Security Audit
June 27, 2024
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Mintra's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from June 6, 2024 to June 10, 2024.
The purpose of this audit is to review the source code of certain Mintra 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 | 1 | - | - | 1 |
Medium | 3 | 1 | - | 2 |
Low | 7 | 1 | - | 6 |
Code Quality | 8 | 3 | - | 5 |
Informational | 1 | - | - | - |
Mintra 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:
1120440473eb94fa01665deace8d4d89d0783c65
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
src/BaseCollection.sol |
|
src/ERC1155Collection.sol |
|
src/ERC721Collection.sol |
|
src/LaunchpadFactory.sol |
|
src/RoyaltySplitter.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
erc20AndErc721IsAnd == false
RoyaltySplitter
contract’s funds can be stuck forever if one receiver can’t hold native token
totalSupply
of tokenId when burning
MIN_PLS
can lead to locked funds in royalty splitter
startOfMint
and endOfMint
100_000
can lead to unwanted high token price
maxMarketPercent
10_000
tokenAddresses
and tokenAmounts
array must have the same length at all case
RoyaltySplitter
_maxTokenId
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. |
erc20AndErc721IsAnd == false
In BaseCollection, there’s additionalFeatures.erc20AndErc721IsAnd
variable that its purpose is for token-gated minting. More specifically:
erc20AndErc721IsAnd
== false, this means the minter only needs to satisfy either ERC20 holding requirement or ERC721 holding requirementerc20AndErc721IsAnd
== true, this means the minter MUST need to satisfy both ERC20 holding requirement and ERC721 holding requirementBut in the code, when erc20AndErc721IsAnd
== false, minter has to satisfy both requirements:
bool hasErc20Check = additionalFeatures.erc20TokenAddresses.length > 0;
bool hasErc721Check = additionalFeatures.erc721TokenAddresses.length > 0;
if (additionalFeatures.erc20AndErc721IsAnd) {
require(isERC20CheckSatisfied && isERC721CheckSatisfied, "Not enough ERC20 and ERC721 tokens");
} else {
if (hasErc20Check) {
require(isERC20CheckSatisfied, "Not enough ERC20 tokens");
}
if (hasErc721Check) {
require(isERC721CheckSatisfied, "Not enough ERC721 tokens");
}
}
If erc20AndErc721IsAnd
== false and erc20TokenAddresses.length
> 0 and erc721TokenAddresses.length
> 0, minter has to satisfy both requirements to pass.
Remediations to Consider
} else {
- if (hasErc20Check) {
- require(isERC20CheckSatisfied, "Not enough ERC20 tokens");
- }
- if (hasErc721Check) {
- require(isERC721CheckSatisfied, "Not enough ERC721 tokens");
- }
+ require(isERC20CheckSatisfied || isERC721CheckSatisfied, "Not enough ERC20 or ERC721 tokens");
}
RoyaltySplitter
contract’s funds can be stuck forever if one receiver can’t hold native token
In distrubuteRoyalty()
function in the RoyaltySplitter
contract, it will send native token to all receivers:
function distrubuteRoyalty(address[] memory erc20Addresses) public payable nonReentrant {
...
if (plsBalance > MIN_PLS) {
(bool success,) = to[i].call{value: amount}("");
require(success, "ETH_TRANSFER_FAILED");
...
}
}
...
}
If there’s an address in the to
array that can’t receive native tokens, it will make the function reverted, which leads to all the funds held (including native token and ERC20s) in this contract being locked forever
Remediations to Consider
This issue can be resolved in different ways:
to
and amountBps
arrays. Note that this requires the contracts to be Ownable, adding centralization risks.It is understood that the to address’s should be a EOA or compatible smart contract address.
totalSupply
of tokenId when burning
ERC1155Collection.sol#L140-L145
In ERC1155Collection
contract, we have a tokenSupply
mapping variable, its purpose is to track the supply of each tokenId:
mapping(uint256 => uint256) public tokenSupply;
As per the definition, the token supply increases on minting and should be decreased on burn.
However, in the burn()
function, tokenSupply
of the burned tokenId will not be decreased:
function burn(uint256 _tokenId, uint256 _amount) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
_burn(msg.sender, _tokenId, _amount);
}
This will lead to the totalSupply
of each tokenId will be higher than the actual underlying total supply of that tokenId. As a consequence, minters are not being able to mint more tokens even though some tokens have been burned
Remediations to Consider
Decrease the token supply by the amount burned.
function burn(uint256 _tokenId, uint256 _amount) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
+ tokenSupply[_tokenId] -= _amount;
_burn(msg.sender, _tokenId, _amount);
}
MIN_PLS
can lead to locked funds in royalty splitter
In RoyaltySplitter.distributeRoyalty
, native tokens can only be distributed to defined recipients when the contract’s balance exceeds MIN_PLS
.
if (plsBalance > MIN_PLS) {
for (uint256 i = 0; i < to.length; i++) {
uint256 amount = plsBalance * amountBps[i] / MAX_BPS;
...
However, the constant MIN_PLS
is set to 1000 ether
.
While this number might not be particular large when deployed on Pulsechain (1 PLS ~ $0.00005), it can get very large when the contracts are considered to be deployed on other chains. This can even lead to locked funds in the contract, as the amount is never reached.
Remediation to Consider
Remove the check plsBalance > MIN_PLS
entirely to allow native balance to be distributed in any case.-
startOfMint
and endOfMint
In BaseCollection’s constructor()
, it doesn’t validate startOfMint
and endOfMint
. If incorrect values are provided, such as startOfMint
> endOfMint
or endOfMint
> block.timestamp
, invalid collections are deployed, where minting of tokens will revert.
function verifyAndProcessPayment(uint256 _amount, address _tokenToPayIn) internal {
...
if (additionalFeatures.isTimed) {
require(
block.timestamp >= additionalFeatures.startOfMint && block.timestamp <= additionalFeatures.endOfMint,
"Minting not started or ended"
);
}
...
Remediations to Consider
Consider adding proper validation for startOfMint
and endOfMint
when the collection are deployed.
100_000
can lead to unwanted high token price
In BaseCollector’s constructor()
, we have this check to make sure the listing price is not too small:
for (uint256 i = 0; i < _priceListToken.length; i++) {
isFreeMint = false;
@> require(_priceListAmount[i] >= 100000, "Price too low");
price[_priceListToken[i]] = _priceListAmount[i];
}
However, some tokens have decimals set to 6
or smaller, which can make the minimum mint price higher than wanted:
100_000
means 0.1 USD100_000
means 1000USDRemediations to Consider
Since this check doesn’t add much value, consider removing this line completely:
- require(_priceListAmount[i] >= 100000, "Price too low");
When we take fees we divide by 10,000 bps to calculate the fee. This is why the restriction is here.
ERC1155Collection.sol#L128-L129
In ERC721Collection and ERC1155Collection contracts, we have [ERC721Collection.mint](http://ERC721Collection.mint)()
, ERC1155Collection.mint()
and ERC1155Collection.devMint()
functions that doesn’t follow CEI (Checks-Effects-Interact) pattern. Meaning that in ERC721Collection.mint
for example, amountMinted
is updated after calling _safeMint
(which makes an external call).
Even though the function uses nonReentrant
modifier, it can still lead to issues like view-only reentrancy attacks.
Remediations to Consider
Follow best practices by adhering to the CEI pattern for aforementioned functions.
maxMarketPercent
EXPLANATION
In LauchpadFactory contract, we have a setMarketPercent()
function:
function setMarketPercent(uint256 _marketPercent) public onlyWizard {
require(_marketPercent >= 0 && _marketPercent <= 369, "Fee not in range");
marketPercent = _marketPercent;
}
Notice here that LaunchpadFactory’s marketPercent
can be set in [0, 369] range. But in BaseCollection’s constructor()
, BaseCollection’s marketPercent
can only be set in [0,225]:
constructor(
string memory _name,
string memory _symbol,
string memory _baseURI,
address[] memory _priceListToken,
uint256[] memory _priceListAmount,
address _feeSplitterAddress,
uint256 _marketPercent,
bytes memory data
) {
...
require(_marketPercent >= 0 && _marketPercent <= 225, "Market percent not in range");
...
As a result, if marketPercent
in LaunchpadFactory is set in [226, 369], LaunchpadFactory can’t deploy any collections.
Remediations to Consider
function setMarketPercent(uint256 _marketPercent) public onlyWizard {
- require(_marketPercent >= 0 && _marketPercent <= 369, "Fee not in range");
+ require(_marketPercent >= 0 && _marketPercent <= 225, "Fee not in range");
marketPercent = _marketPercent;
}
10_000
In LaunchpadFactory’s deployCollection
, there is the following check on the maximum tokenId allowed:
require(_maxTokenId > 0 && _maxTokenId <= 10000, "maxTokenId not in range");
While a lot of NFT projects use a collection quantity of 10_000
tokens, there can be valid use cases where one wants to create collections consisting of more than 10_000
tokens, both for ERC721 and ERC1155. Restricting the maxTokenId
to 10_000 unnecessarily limits the use case of those collections.
Remediation to Consider
Remove the upper limit of 10_000
for _maxTokenId
.
In the LaunchpadFactory
contract, we have a collections
mapping variable:
// mapping of collection addresses to their types
mapping(address => CollectionType) public collections;
Enum CollectionType
looks like this:
enum CollectionType {
ERC721,
ERC1155
}
The default value for CollectionType
is ERC721, which leads to reporting misleading information when calling the view function of collections
mapping for addresses that have not been deployed via the factory.
Remediations to Consider
Use a spacer value as a default type
enum CollectionType {
+ NO_OP,
ERC721,
ERC1155
}
Since the code might be deployed to other chains, it should be considered to be reorgs protected.
About reorgs: also known as block reorganization, reorgs occur when network nodes fall out of sync with each other, and two distinct chains of blocks are produced concurrently. This may be due to a bug, network latency, or even malicious activity. When nodes sync once again, one canonical version of the chain is kept, and the blocks included in the invalid ‘fork’ are ignored.
Polygon historically got 157-block reorg, which is ~ 5 minutes: https://protos.com/polygon-hit-by-157-block-reorg-despite-hard-fork-to-reduce-reorgs/
Reorg can happen when a new Collection contract is deployed, and plenty of minters are trying to mint as much as possible. The reorgs that happen will make the Collection contract get undeployed. Malicious users take advantage of that to deploy new Collection contracts with the same address, minter didn’t notice that and still trying to mint NFT, but now malicious users take profit for that instead of the victim project.
Remediations to Consider
The root cause of this issue is that people can have the ability to deploy the contract with identical addresses. This can be solved by using CREATE2 and a carefully done salt to deploy the contract
CREATE is a deploying method that the deployed contract address depend on the msg.sender
's address and msg.sender
's nonce:
new_address = hash(sender, nonce)
CREATE2 is a deploying method that the deployed contract address depend on the msg.sender
's address, an arbitrary salt, and the deployed contract’s bytecode:
new_address = hash(0xFF, sender, salt, bytecode)
Further reading: https://docs.openzeppelin.com/cli/2.8/deploying-with-create2
To make sure deployers can’t deploy a contract with collided address with each other, we must include deployer’s address into the salt
ERC721Collection.sol#L130-L135
ERC1155Collection.sol#L140-L145
In ERC721Collection
contract, burners have to own the token in order to burn it. This can lead burners who have permission from other addresses, or have permission to use the burning token, to transfer it to their own wallet in order to burn()
:
function burn(uint256 tokenId) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
// msg.sender must be token owner
require(ownerOf(tokenId) == msg.sender, "Token not owned");
_burn(tokenId);
}
The same issue appears in ERC1155Collection
, the burner who has permission from another address need to transfer the tokens to their own wallet first in order to burn it:
function burn(uint256 _tokenId, uint256 _amount) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
// msg.sender must be token owner
require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
_burn(msg.sender, _tokenId, _amount);
}
Remediations to Consider
For ERC721Collection
, consider putting burn permission for burners that have an allowance of the burning token, or burners that have permission to other addresses that hold the burning token. In ERC721A
contract, there’s a function function _burn(uint256 tokenId, bool approvalCheck)
that we can use for this case (ERC721A.sol#L1130). If approvalCheck == true
, it will check if msg.sender
has the allowance of the burning token or msg.sender
has permission to other addresses that hold the burning token:
function burn(uint256 tokenId) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
- require(ownerOf(tokenId) == msg.sender, "Token not owned");
- _burn(tokenId);
+ _burn(tokenId, true);
}
For ERC1155Collection
, consider putting burn permission for the burner having permission to other addresses that hold the burning token.
- function burn(uint256 _tokenId, uint256 _amount) external {
+ function burn(uint256 _tokenId, uint256 _amount, address _burner) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");
- require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
+ require(_burner == msg.sender || isApprovedForAll(_burner, msg.sender), "Token not owned or approved");
- _burn(msg.sender, _tokenId, _amount);
+ _burn(_burner, _tokenId, _amount);
}
tokenAddresses
and tokenAmounts
array must have the same length at all case
In BaseCollection, the constructor checks that erc20TokenAddresses == erc20TokenAmounts
and erc721TokenAddresses == erc721TokenAmounts
. However, this check is only done in the case erc20AndErc721IsAnd == true
, but should be done in any case.
Remediations to Consider
constructor(
string memory _name,
string memory _symbol,
string memory _baseURI,
address[] memory _priceListToken,
uint256[] memory _priceListAmount,
address _feeSplitterAddress,
uint256 _marketPercent,
bytes memory data
) {
...
- if (_additionalFeatures.erc20AndErc721IsAnd) {
require(
_additionalFeatures.erc20TokenAddresses.length == _additionalFeatures.erc20TokenAmounts.length,
"ERC20 list mismatch"
);
require(
_additionalFeatures.erc721TokenAddresses.length == _additionalFeatures.erc721TokenAmounts.length,
"ERC721 list mismatch"
);
+ if (_additionalFeatures.erc20AndErc721IsAnd) {
require(_additionalFeatures.erc20TokenAddresses.length > 0, "ERC20 list empty");
require(_additionalFeatures.erc721TokenAddresses.length > 0, "ERC721 list empty");
}
...
}
RoyaltySplitter
Currently, the only way to send native token to RoyaltySplitter
contract is by calling to distrubuteRoyalty()
function:
function distrubuteRoyalty(address[] memory erc20Addresses) public payable nonReentrant {
...
}
Remediations to Consider
Consider adding a fallback
or receive
function to the RoyaltySplitter
contract in order to receive native token directly.
In BaseCollection
contract, the collection owner can configure the contract to be ERC20 and ERC721 token-gated for minters when minting tokens. But currently, the contract doesn’t support ERC1155 token-gated.
Remediations to Consider
For a more flexible contract, consider adding ERC1155 token-gated feature
In BaseCollection
contract, getAdditionalFeatures()
, onAllowList()
, getPrice()
view functions can be removed because due to the public state variables, we have additionalFeatures()
, allowlist()
, price()
view functions that return exactly the same value.
ERC721Collection.sol#L169-L178
In ERC721Collection
contract, name()
and symbol()
don’t need to be overriden. Because ERC721Collection's myName
and mySymbol
have the same value as ERC721A’s _name
and _symbol
, ERC721A’s and ERC721Collection’s name()
and symbol()
functions return the same value
Remediations to Consider
Consider removing those unnecessary view functions
On the following occurrences, a number is used instead of a constant:
Remediations to Consider
To improve readability, consider using constants for the number mentioned above.
_maxTokenId
In ERC721Collection, _maxTokenId
is passed to the constructor and defined as follows:
* @param _maxTokenId The maximum supply of tokens that can be minted for the collection.
However, the name _maxTokendId
implies, that this is the maximum number of token id that can be minted, rather than the maximum number of supply. It is verified in the mint
function:
require(supply + _amount <= maxTokenId, "Max maxTokenId reached");
As per above, maxTokenId
is treated as the maximum allowed supply. This can create confusions amongst users, as tokens can be minted with tokenIds > maxTokenId
.
Remediation to Consider
Rename _maxTokenId
to a more meaningful name such as _maxTokenMinted
.
In ERC721Collection and ERC1155Collection, supportsInterface()
can be refactored to me more readable:
+ return ERC2981.supportsInterface(interfaceId) || ERC721A.supportsInterface(interfaceId);
- return ERC2981.supportsInterface(interfaceId) ? true : ERC721A.supportsInterface(interfaceId);
Currently, in the BaseCollection.sol#L101 constructor()
, isFreeMint
variable gets set to false
multiple times every loop:
for (uint256 i = 0; i < _priceListToken.length; i++) {
isFreeMint = false;
require(_priceListAmount[i] >= 100000, "Price too low");
price[_priceListToken[i]] = _priceListAmount[i];
}
Move isFreeMint = false;
outside of the loop to save gas.
In BaseCollection.withdraw()
function, a payable
tag is not necessary, since this function doesn’t need to hold native token:
- function withdraw(address _tokenAddress) external payable onlyOwner {
+ function withdraw(address _tokenAddress) external onlyOwner {
...
In BaseCollection.verifyAndProcessPayment()
, required ERC20 will be pulled from msg.sender
to the contract first, and then distributed to feeSplitterAddress
and owner()
. We can reduce one external call here by sending ERC20 directly from msg.sender
to feeSplitterAddress
and owner()
- IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, address(this), totalCost);
- IERC20(_tokenToPayIn).safeTransfer(feeSplitterAddress, launchpadFee);
- IERC20(_tokenToPayIn).safeTransfer(owner(), totalCost - launchpadFee);
+ IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, feeSplitterAddress, launchpadFee);
+ IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, owner(), totalCost - launchpadFee);
In BaseCollection, import of Strings and ReentrancyGuard is not used and can be removed
In BaseCollection, for isERC20CheckSatisfied and isERC721CheckSatisfied, it is not necessary to set default value.
Inaccurate natspec in ERC1155Collection:
_maxSupplytoken
ERC721Collection and ERC1155Collection use the var maxMintableAmount
to restrict the number of tokens allowed to be minted per address. However, this check can be easily bypassed, as a malicious user can just create as much addresses as they want.
Additionally, both ERC721 and ERC1155 collections implement token gating to restrict minting of tokens only to addresses holding certain tokens. This can also be easily bypassed by users utilizing flashloans to borrow assets.
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 Mintra 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.