Closed thibautvdu closed 1 year ago
yuriy77k
commented
1 year ago The audit fee is 1000 USDT. You may send USDT (ERC20 or BEP20) to: 0x6317c6944bd1cD3932d062cce39d7Fd602119529 (valid for Ethereum and Binance Smart Chain)
The estimated auditing time - is 10 days after payment.
thibautvdu
commented
1 year ago Many thanks @yuriy77k , payment done : https://etherscan.io/tx/0x430ab3cc7111055169894064902527107d7603f3622e5ffcac61501a77c26b6c
Please note that a minor change was made (two functions removed) to reflect the conclusion of a prior audit.
https://github.com/MechaChain/MechaChain-Smart-Contracts/blob/main/contracts/ERC721/MechaPilots2219V1.sol commit : 9cb6aee
yuriy77k
commented
1 year ago @thibautvdu Thank you, we start auditing.
yuriy77k
commented
1 year ago https://github.com/MechaChain/MechaChain-Smart-Contracts/blob/main/contracts/ERC721/MechaPilots2219V1.sol commit : 9cb6aee
I can't find commit 9cb6aee. the latest commit which I see is 8f3b7b2 (2 days ago).
Please, confirm if it is correct.
thibautvdu
commented
1 year ago Hello @yuriy77k This is correct; this file wasn't changed in 8f3b7b2 which is a documentation (readme) update
Thanks !
yuriy77k
commented
1 year ago MechaChain - 2219 NFT collection smart contract security audit report performed by Callisto Security Audit Department
Commit: 9cb6aee43a03610751f4599ceff3631e720417bb
In total, 1 issues were reported, including:
0 high severity issues.
1 medium severity issue.
0 low severity issues.
In total, 18 notes were reported, including:
7 notes.
11 owner privileges.
No critical security issues were found.
Internal function _roundMint() is used by functions mintWithValidation() and mint() which allow the user to mint n amount of tokens for an active round with or without validator signature respectively. The function _roundMint() accepts native chain currency ETH as payment for minting tokens, the price of token(s) is computed using the function roundPrice().
The statement in the function _roundMint() checks if the total price of the token is less than the amount of eth attached to the transaction require(roundPrice(roundId) * amount <= msg.value, "Wrong price");.
As the price of the token for the round is reduced over time if the round validator exists else the price remains constant.
If the msg.value for the transaction is greater than the value of tokens computed using the function roundPrice(), the excess ETH amount does not get refunded back to the User, and the contract does not implement any measures for users to claim back excess funds. Leading to users overpaying for the token(s).
Change the following statement to the following to accept the exact ETH amount for the token(s) value
require(roundPrice(roundId) * amount == msg.value, "Wrong price");(or)
Implement pay back excess funds to user:
uint256 correctPrice = roundPrice(roundId) * amount;
// Correct price
require(correctPrice <= msg.value, "Wrong price");
if (correctPrice < msg.value)
payable(msg.sender).transfer(msg.value - correctPrice);
}Function setupMintRound() allows the owner to modify existing round parameters after the round has been active.
If a round is reinitialized during or after the round period.
Consider adding a check to prevent modification to round parameters for active or completed rounds.
if(round.startTime != 0) {
require(round.startTime > block.timestamp, "Round in progress")
}revealToken() allows authorized addresses by the owner to reveal the user's token using the signature of a URI_UPDATER_ROLE which is moderated by the admin role using openzepplin's Access Control Upgradeable contract's implementation.airdrop() allows owner to airdrop token(s) to any Externally owned account (EOA).setupMintRound() allows the owner to set up an arbitrary number of rounds for minting tokens (public and whitelisted).pause()/unpause() allows the owner to disable/enable mints, transactions, and burns for the users.setBaseURI() and setBaseExtension() allows owners to modify/set baseURI and base metadata extension for the unrevealed tokens respectively.setBurnable() allows the owner to configure whether users can burn token(s) or not.setMaxMintsPerWallet() allows the maximum number of tokens that can be minted by an Externally owned account (EOA) for a public round.withdraw() allows the owner to transfer ETH from the contract.withdrawTokens() allows owner to transfer/recover ERC-20 tokens from the contract.Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract.
The function mintWithValidation() allows the user to mint token(s) with a signature provided by an authorized validator. If the maxMint is greater than the amount (no. of token(s)) that can be minted by a user. This would allow users to mint multiple times using the same signature until the maxMint is reached for the round.
Consider adding a nonce with mapping to the roundId for the validator for the Externally owned account (EOA), and validate the nonce during signature verification to prevent signature replay.
ReentrancyGuardUpgradeable is imported into the contract but is not used.
Consider removing the unused import.
Since Solidity v0.4.0 functions can be explicitly marked as payable to accept ETH, the function receive() is implemented in the contract to accept ETH directly to the contract and is not necessary for the contract's logic.
Consider removing the receive() function to revert unintentional transfers.
As per dev documentation for the function, setupMintRound() the roundId for the round can be zero but the required statement prevents the roundId from being zero.
Consider updating the documentation or removing the check as per requirements.
The comment says: The number of seconds to wait between each decreasing (must be > 900), but there is no check of that. Also looks like comments confuse description for priceDecreaseTime and priceDecreaseAmount
Add requirement:
require(priceDecreaseTime == 0 || priceDecreaseTime > 900);totalSupply() returns the value of minted tokens minus burned tokens but totalSupplyByFaction only returns minted tokens.
Add counter totalBurnedByFaction or reduce totalSupplyByFaction using tokenFaction map to get the token´s faction.
It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract. It also helps in saving gas costs.
Few functions and Enums in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well.
There are 2 instances where this was violated:
Here _mint is called before the state updates of _totalMinted and _totalMintedByFaction
Here ownerToRoundTotalMinted is updated after the _safeMintCall
CREATE (0xf0) opcode is assigned following this scheme keccak256(rlp([sender, nonce])). Therefore you need to use the same address that was originally used at the main chain to deploy the mock contract at a transaction with the nonce that matches that on the original chain. Example: If you have deployed your main contract with address 0x010101 at your 2021th transaction then you need to increase your nonce of 0x010101 address to 2020 at the chain where your mock contract will be deployed. Then you can deploy your mock contract with your 2021th transaction, and it will receive the same address as your mainnet contract.The audited smart contract must not be deployed. Reported issues must be fixed prior to the usage of this contract.
It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.
Severity was changed from "critical" to "medium", because this issue affect only on users who send bigger value than should be. In case price of the token changes after user's payment it's not a "critical" issue, because user agreed to buy token by this price.
Severity was changed from "medium" to "note", because it does not change purpose of validation. Anyway user can't buy more tokens than maxMint. Implementation of nonce will require to use additional storage slot in the smart contract to store already processed nonce which will increase transaction gas cost without any advantage for security.
It's not an issue. Here decreaseBy calculates as number of decreaseTime period multiply by decreaseAmount (decrease amount per period).
Not an issue. The requirement considers entire round duration. it starts
Not an issue. The constructor is called _disableInitializers();.
It is not an issue. If maxMintsPerWallet is too big, users can mint tokens using amount of tokens that can be minted.
thibautvdu
commented
1 year ago Hi @yuriy77k ,
Thanks a lot for the audit. As mentioned in my e-mail, we implemented most of your suggestions, and also had to slightly modify the contract by implementing the new requirement of OpenSea for on-chain enforcement. We just implemented their interface : https://github.com/ProjectOpenSea/operator-filter-registry
And the changes we did based on your audit :
Is it possible to update the audit taking into account those corrections and this new feature ? Please let me know the conditions. code : https://github.com/MechaChain/MechaChain-Smart-Contracts/blob/main/contracts/ERC721/MechaPilots2219V1.sol commit : ad14bd0f2e2eb83d7453221362004d88c3878acf
yuriy77k
commented
1 year ago Hi @thibautvdu We are working on reaudit
thibautvdu
commented
1 year ago @yuriy77k thanks ; looking forward to the update 👍
thibautvdu
commented
1 year ago Hello @yuriy77k , any update ?
yuriy77k
commented
1 year ago MechaChain - 2219 NFT collection smart contract security audit report performed by Callisto Security Audit Department
Commit: 5a161c0c58f6cbac8e475c1fc6378099997a94e4
There is import of file UpdatableOperatorFiltererUpgradeable.sol but it does not exist in provided repository so it was excluded from audit.
Contracts from OpenZeppelin library that were already audited.
In total, 1 issues were reported, including:
0 high severity issues.
0 medium severity issue.
1 low severity issues.
In total, 14 notes were reported, including:
2 notes.
12 owner privileges.
No critical security issues were found.
Internal function _roundMint() is used by functions mintWithValidation() and mint() which allow the user to mint n amount of tokens for an active round with or without validator signature respectively. The function _roundMint() accepts native chain currency ETH as payment for minting tokens.
The statement in the function _roundMint() checks if the total price of the token is less than the amount of eth attached to the transaction require(round.price * amount <= msg.value, "Wrong price");.
If the msg.value for the transaction is greater than the value to of specific amount of tokens, the excess ETH amount does not get refunded back to the User, and the contract does not implement any measures for users to claim back excess funds. Leading to users overpaying for the token(s).
Change the following statement to the following to accept the exact ETH amount for the token(s) value
require(roundPrice(roundId) * amount == msg.value, "Wrong price");(or)
Implement pay back excess funds to user:
uint256 correctPrice = round.price * amount;
// Correct price
require(correctPrice <= msg.value, "Wrong price");
if (correctPrice < msg.value)
payable(msg.sender).transfer(msg.value - correctPrice);
}Function setupMintRound() allows the owner to modify existing round parameters after the round has been active.
If a round is reinitialized during or after the round period.
Consider adding a check to prevent modification to round parameters for active or completed rounds.
if(round.startTime != 0) {
require(round.startTime > block.timestamp, "Round in progress")
}revealToken() allows authorized addresses by the owner to reveal the user's token using the signature of a URI_UPDATER_ROLE which is moderated by the admin role using openzepplin's Access Control Upgradeable contract's implementation.airdrop() allows owner to airdrop token(s) to any Externally owned account (EOA).setupMintRound() allows the owner to set up an arbitrary number of rounds for minting tokens (public and whitelisted).pause()/unpause() allows the owner to disable/enable mints, transactions, and burns for the users.setBaseURI() and setBaseExtension() allows owners to modify/set baseURI and base metadata extension for the unrevealed tokens respectively.setBurnable() allows the owner to configure whether users can burn token(s) or not.setMaxMintsPerWallet() allows the maximum number of tokens that can be minted by an Externally owned account (EOA) for a public round.withdraw() allows the owner to transfer ETH from the contract.withdrawTokens() allows owner to transfer/recover ERC-20 tokens from the contract.setTokenURI() and setTokenURIPerBatch() allow authorized address with URI_UPDATER_ROLE set URI for any tokenssetDefaultRoyalty() and deleteDefaultRoyalty() allow owner to set/delete royalty receiver and percentage.Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract.
Since Solidity v0.4.0 functions can be explicitly marked as payable to accept ETH, the function receive() is implemented in the contract to accept ETH directly to the contract and is not necessary for the contract's logic.
Consider removing the receive() function to revert unintentional transfers.
It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract. It also helps in saving gas costs.
Few functions and Enums in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well.
CREATE (0xf0) opcode is assigned following this scheme keccak256(rlp([sender, nonce])). Therefore you need to use the same address that was originally used at the main chain to deploy the mock contract at a transaction with the nonce that matches that on the original chain. Example: If you have deployed your main contract with address 0x010101 at your 2021th transaction then you need to increase your nonce of 0x010101 address to 2020 at the chain where your mock contract will be deployed. Then you can deploy your mock contract with your 2021th transaction, and it will receive the same address as your mainnet contract.The audited smart contract can't be considered as safe since UpdatableOperatorFiltererUpgradeable.sol was not available for audit. In the audited code only low severity issue was found.
It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.
https://gist.github.com/yuriy77k/e4f7eb6aa793f47c5af7eed20c27a933
Audit request
2219 is the new ERC712 NFT collection of MechaChain ; the source code shares many similarities with with our previous audited contract : https://github.com/EthereumCommonwealth/Auditing/issues/642
The fixed supply is divided in two factions (two 'type' of collectibles) and the user mint NFTs from a given faction.
Mint The contract allow us to configure 'rounds', which we planned as follow :
All rounds combined, the number of minted NFTs cannot exceed the total supply of both factions combined.
The mint use on-chain randomness for fairness.
Unrevealed NFT The unrevealed NFTs are already unique ERC721 tokens. They will contain a 'dna' field in their metadata, which is the IPFS hash of the future revealed token. This is to guarantee to our users that the reveal is fair and untempered since the initial mint use on-chain randomness.
Reveal Each NFT can be, after a date configured by the smart contract admin, revealed by its owner. There is no 'global reveal' and each reveal is individual. The reveal function is called by the user with a signature provided by our server. The signature contains the revealed URI of the token, which is the dna IPFS hash of the unrevealed token's metadata.
Only then, the server will upload the new json file and medias.
Source code
https://github.com/MechaChain/MechaChain-Smart-Contracts/blob/main/contracts/ERC721/MechaPilots2219V1.sol
Payment plan
... Write [x] at the checkbox of the payment plan that suits your needs ...
Disclosure policy
Standard disclosure policy.
Contact information (optional)
Contact information : aquifere06panels@icloud.com
Project website / medias : https://mechachain.io https://twitter.com/2219project
Platform
ETH