A security review of the Metalabel smart contract protocol was done by Gogo.
This audit report includes all the vulnerabilities, issues and code improvements found during the security review.
"Audits are a time, resource and expertise bound effort where trained experts evaluate smart contracts using a combination of automated and manual techniques to find as many vulnerabilities as possible. Audits can show the presence of vulnerabilities but not their absence."
- Secureum
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High | Critical | High | Medium |
| Likelihood: Medium | High | Medium | Low |
| Likelihood: Low | Medium | Low | Low |
- High - leads to a significant material loss of assets in the protocol or significantly harms a group of users.
- Medium - only a small amount of funds can be lost (such as leakage of value) or a core functionality of the protocol is affected.
- Low - can lead to any kind of unexpected behaviour with some of the protocol's functionalities that's not so critical.
- High - attack path is possible with reasonable assumptions that mimic on-chain conditions and the cost of the attack is relatively low to the amount of funds that can be stolen or lost.
- Medium - only conditionally incentivized attack vector, but still relatively likely.
- Low - has too many or too unlikely assumptions or requires a huge stake by the attacker with little or no incentive.
- Critical - client must fix the issue.
- High - client must fix the issue.
- Medium - client should fix the issue.
- Low - client could fix the issue.
| Project Name | Metalabel |
| Repository | https://github.com/metalabel/metalabel-contracts-v1_1 |
| Commit hash | fb04291dfdf7114bbec12ef5ec30b4135eac4878 |
| Documentation | link |
| Methods | Manual review |
| Severity | Count |
|---|---|
| Critical risk | 0 |
| High risk | 0 |
| Medium risk | 5 |
| Low risk | 7 |
| Informational | 6 |
| File | SLOC |
|---|---|
| Contracts (5) | |
| contracts/Memberships.sol | 261 |
| contracts/MembershipsFactory.sol | 51 |
| contracts/ControllerV1.sol | 172 |
| contracts/engines/DropEngineV2.sol | 299 |
| contracts/RevenueModuleFactory.sol | 80 |
| Abstracts (0) | |
| Interfaces (0) | |
| Total (5) | 863 |
[M-1] Revenue can be forwarded to the null address when price = 0 and revenueRecipient == address(0), but priceDecayPerDay > 0
If a drop price is configured to decay to 0 and at the same time decayStopTimestamp is > block.timestamp (e.g. user want the initial price to be 5 ETH and after 5 days to go down to 0), the revenueRecipient is restricted to be address(0). This will lead to transferring all the revenue generated from a given drop to the null address.
Modify the input validation in DropEngineV2.sol so users can configured sequences with price decaying to 0 and a revenueRecipient != address(0).
You can add the following check:
- // Ensure that if a price is set, a recipient is set, and vice versa
- if (
- (dropData.price == 0) != (dropData.revenueRecipient == address(0))
- ) {
- revert InvalidPriceOrRecipient();
- }
+ // Ensure that if a price or decaying price is set, a recipient is set, and vice versa
+ if (
+ (dropData.price == 0 && dropData.priceDecayPerDay == 0) !=
+ (dropData.revenueRecipient == address(0))
+ ) {
+ revert InvalidPriceOrRecipient();
+ }[M-2] Insufficient input validation - admin can transfer non-existent membership tokens from address(0) in Memberships.sol
Admin should not be able to transfer membership from the null address since it will result in DoS of the mint-ing functionality when an id >= minted is passed to Memberships.adminTransferFrom.
Add the following check in Memberships.adminTransferFrom:
if (to == address(0)) revert InvalidTransfer();
+ if (from == address(0)) revert InvalidTransfer();
if (balanceOf(to) != 0) revert InvalidTransfer();
if (from != _tokenData[id].owner) revert InvalidTransfer();If for some reason the dropData.revenueRecipient is set to an account that can not receive funds (e.g. a contract that doesn't have a payable fallback or receive function) nobody will be able to mint records from the given drop.
Follow the withdrawal pattern for revenue collection. This is done for fee collection by the owner.
- // Forward ETH to the revenue recipient
- if (amountToForward > 0) {
- (bool success, ) = drop.revenueRecipient.call{
- value: amountToForward
- }("");
- if (!success) revert CouldNotTransferEth();
- }
+ if (amountToForward > 0) {
+ revenueToReceive[drop.revenueRecipient] += amountToForward;
+ }NOTE
The above will require changes to be made in the DropEngineV2.transferFeesToOwner function as well.
[M-4] Single-step ownership transfer can potentially lead to loosing all primary sales fees in DropEngineV2
Usually this would be considered a low severity issue, but in the case of the new DropEngineV2 contract the owner is the only account that has access to the funds stored in the contract (the collected primary sales fees) so it is critical to ensure (on smart contracts level) it can't be mistakenly set to a non-existent ethereum account.
Implement two-step ownership transfer for the DropEngineV2 contract. Similar is done for the node ownership transfers in NodeRegistry.
Currently there's is a centralization issue with the privileged owner of DropEngineV2. As stated in a comment in DropEngineV2.configureSequence, you don't allow the caller to control the primary sale fee, instead it is only set by the contract owner. That means the owner decides how much to take from each mint order price. The owner can set the primarySaleFeeBps to 10000 (the max bps) and therefore take all of the revenue from each next drop leaving the dropData.revenueRecipient with nothing.
Consider adding a constant (e.g. MAX_PRIMARY_SALE_FEE_BPS) that restricts the owner of the engine to set the primary sale fee to more than a given percent:
+ uint16 constant MAX_PRIMARY_SALE_FEE_BPS = 3000; // 30%
/// @notice Set the primary sale fee for all drops configured on this
/// engine. Only callable by owner
function setPrimarySaleFeeBps(uint16 fee) external onlyOwner {
- if (fee > 10000) revert InvalidPrimarySaleFee();
+ if (fee > MAX_PRIMARY_SALE_FEE_BPS) revert InvalidPrimarySaleFee();
primarySaleFeeBps = fee;
emit PrimarySaleFeeSet(fee);
}The _sequence.sealedAfterTimestamp and _sequence.sealedBeforeTimestamp variables in Collection.configureSequence define a timebound sequence. When a drop has a decaying price the decay stop timestamp should logically be before the end of the records minting window. Otherwise the dropData.price will never be reached in the given window because of the checks in Collection._validateSequence.
Verify that decayStopTimestamp is <= sealedAfterTimestamp in Collection.configureSequence.
There are 2 external calls being made in the mint function of DropEngineV2 and both of the are to arbitrary / user-controlled addresses. Although the CEI pattern is observed correctly in this function, it is also a good practice to implement the withdrawal pattern as well as to use a re-entrancy guard.
Consider using the nonReentrant modifier from OZ's library in the mint function in DropEngineV2.
Implement the withdrawal pattern for withdrawing the drop revenue.
The following external low-level call is made in DropEngineV2.mint:
(bool success, ) = drop.revenueRecipient.call{
value: amountToForward
}("");
The whole transaction gas is forwarded to this call which means that the drop.revenueRecipient can use it for any purpose that it finds incentive for. Each minter will have to pay an arbitrary (eventually a huge) amount of gas for the logic that the revenueRecipient may implement if it is a smart contract account with a payable receive or fallback function.
Follow the withdrawal pattern for revenue collection. This is done for fee collection by the owner.
- // Forward ETH to the revenue recipient
- if (amountToForward > 0) {
- (bool success, ) = drop.revenueRecipient.call{
- value: amountToForward
- }("");
- if (!success) revert CouldNotTransferEth();
- }
+ if (amountToForward > 0) {
+ revenueToReceive[drop.revenueRecipient] += amountToForward;
+ }NOTE
The above will require changes to be made in the DropEngineV2.transferFeesToOwner function as well.
The requirements in DropEngineV2 are that the owner of the contract decides what the primarySaleFeeBps should be for each drop. This is enforced in the code in DropEngineV2.configureSequence by overriding the value that the user has passed for this parameter. It could be dangerous if the user is not aware of this behaviour.
As stated in a comment by the dev the better approach would be to require the caller to pass in the current fee to assert the caller is aware of this behavior
ERC721 token transfers in Memberships.sol are not allowed by simply overriding the transferFrom function from solmate and reverting on each call. The same should be done for Memberships.approve since there is no point of approving tokens that can not be transferred later. This might prevent potential future problems.
Add the following code in Memberships.sol:
+ /// @notice Approval is not allowed on this token.
+ function approve(address, uint256) public pure override {
+ revert ApprovalNotAllowed();
+ }
+ /// @notice Approval is not allowed on this token.
+ function setApprovalForAll(address, bool) public pure override {
+ revert ApprovalNotAllowed();
+ }[L-6] The drop revenue recipient can mint infinite amount of records in a single transaction for free if primarySaleFeeBps = 0
The current design of minting records in a drop allows the revenue recipient to mint whatever amount of records they want practically for free. Since the dropData.revenueRecipient receives all funds for the sales (when primarySaleFeeBps = 0) they can simply mint again and again which will inflate the total supply of records. This may not be expected by users.
Not sure how this could be prevented or if it should be prevented at all, but I thought it is worth mentioning.
Currently there is not relationship between the dropData.priceDecayPerDay, dropData.decayStopTimestamp and dropData.price. A user can set the price to decay for example from 100 ETH to 0.00001 ETH in just 1 day which doesn't seem to be a good UX design.
Consider implementing some restrictions for priceDecayPerDay and decayStopTimestamp.
Copying the config.waterfallRecipients to memory in RevenueModuleFactory.deployRevenueModule can be done in more efficient way using inline-assembly.
Consider the 2 following recommendations:
- Saves ~2.5k gas
address[] memory recipients;
assembly {
// Set recipients.length = config.waterfallRecipients.length + 1.
mstore(recipients, add(calldataload(0x124), 1))
// Copy all elements from calldata to the recipients memory array.
calldatacopy(add(recipients, 0x20), 0x144, mul(calldataload(0x124), 0x20))
// Add the split address at the end of the waterfall recipients array.
mstore(add(add(recipients, 0x20), mul(calldataload(0x124), 0x20)), split)
}
- Saves ~2k gas
address[] memory recipients = config.waterfallRecipients;
assembly {
// Add the split address at the end of the memory recipients array.
mstore(add(add(recipients, 0x20), mul(mload(recipients), 0x20)), split)
// Set recipients.length = config.waterfallRecipients.length + 1.
mstore(recipients, add(mload(recipients), 1))
}
When you have multiple input validation checks the less expensive ones should come before the more expensive so in case of a revert less gas is wasted. In Memberships.mintMemberships there are 2 checks in the for loop - whether the result from MerkleProofLib.verify is true and whether the to address has no membership tokens already. The MerkleProofLib.verify is expensive operation so the balanceOf(mints[i].to) > 0 should happen before that.
Change code to:
+ // enforce at-most-one membership per address
+ if (balanceOf(mints[i].to) > 0) revert InvalidMint();
bool isValid = MerkleProofLib.verify(
mints[i].proof,
membershipListRoot,
keccak256(abi.encodePacked(mints[i].to, mints[i].sequenceId))
);
- // enforce at-most-one membership per address
- if (!isValid || balanceOf(mints[i].to) > 0) revert InvalidMint();
+ if (!isValid) revert InvalidMint();- contracts/Memberships.sol#L420-L462
- contracts/engines/DropEngineV2.sol#L397-L523
- contracts/engines/DropEngine.sol#L266-L272
Since solidity version 0.8.12 was released the new method string.concat is suggested to be used instead of abi.encodePacked for concatenating strings. This is done on multiple places around the current codebase especially in DropEngineV2.
Use string.concat instead of abi.encodePacked for concatenating strings.
Right now, when someone wants to mint multiple records from a given drop they will call DropEngineV2.mint which iterates a count number of times and calls collection.mintRecord each time. In Collection.mintRecord we can see that on each time the function is called there is a verification procedure for the passed sequence and the msg.sender (the engine). This incurs overhead when multiple records are minted at once via DropEngineV2.mint because _validateSequence is called each time.
Consider adding a new function (e.g. batchMintRecord) in the Collection.sol contract that will mint multiple records at once and will _validateSequence only once.
NOTE
Only the checks on lines 286 and 291-295 should be performed once per batchMintRecord. The remaining supply check should consider the minting amount (the count).
The max value for variables of type uint40 is 2**40-1 or in the context of a timestamp this would be after the year of 36000. Currently variables that hold a timestamp value such as sealedBeforeTimestamp, sealedAfterTimestamp and decayStopTimestamp are of type uint64. Using uint40 instead of uint64 is more appropriate and might help with structs packing.
Use uint40 for sealedBeforeTimestamp, sealedAfterTimestamp and decayStopTimestamp instead of uint64.
The CreateMembershipsConfig and CreateCollectionConfig structs are almost identical so a single struct can be used instead of 2 separate. Even the comment for CreateMembershipsConfig is copied from CreateCollectionConfig.
Consider using a single struct like for example ResourceConfig instead of CreateMembershipsConfig and CreateCollectionConfig.