JMaria conducted a security review of the [undisclosed name] protocol, which was limited to a specific time frame. The review primarily focused on the security aspects of the application's implementation.
JMaria, also known as devScrooge on social networks, is a Telecommunications and Blockchain Engineer who currently works full-time as a DeFi developer and also performs security audits for smart contracts. He strives to make valuable contributions to the blockchain ecosystem and its protocols by dedicating his time and effort to conducting security research and performing thorough security reviews.
Socials: Webpage Twitter LinkedIn Github ResearchGate
The audited protocol main functionallity is to allow user to mint and transfer NFTs automatically using their frontend.
| Vulnerability Level | Classification |
|---|---|
| Critical | Easily exploitable by anyone, causing loss/manipulation of assets or data. |
| High | Arduously exploitable by a subset of addresses, causing loss/manipulation of assets or data. |
| Medium | Inherent risk of future exploits that may or may not impact the smart contract execution due to current or future implementations. |
| Low | Minor deviation from best practices. |
| Gas | Gas Optimization |
The following smart contracts were in scope of the audit:
- [Undisclosed name]
The following number of issues were found, categorized by their severity:
- Critical: 2 issues
- High: 1 issues
- Medium: 0 issues
- Low: 1 issues
- Informational: 1 issues
- Gas: 6 issues
| ID | Title | Severity |
|---|---|---|
| C-01 | Any user can mint unlimited amount of tokens for free | Critical |
| C-02 | Seller is receiving funds without transfering any token | Critical |
| H-01 | Reentrancy attack is possible | High |
| L-01 | Unsafe use of .transfer for sending ETH |
Low |
| I-01 | Functions not used internally could be marked external | Informational |
| GAS-01 | Cache array length outside of loop | Gas |
| GAS-01 | Cache array length outside of loop | Gas |
| GAS-02 | Use calldata instead of memory for function arguments that do not get mutated | Gas |
| GAS-03 | Use Custom Errors | Gas |
| GAS-04 | Don't initialize variables with default value | Gas |
| GAS-05 | Pre-increments and pre-decrements are cheaper than post-increments and post-decrements | Gas |
| GAS-06 | Increments can be unchecked in for-loops |
Gas |
The function mintTokenByCreator can be called by any user without any restriction and mint an unlimited amount of tokens to the caller without paying any amount for them.
The function mintTokenByCreator should be used only by the creator as the NatSpec comment indicates:@notice only creator can mint private nfts. However, the function does not include any restriction access modifier, there fore any user can execute the function and mint tokens themselves for free.
function mintTokenByCreator(
uint256[] memory tokenIds,
string[] memory tokenUris
) public checklengthOfTokenIdsAnduris(tokenIds, tokenUris) {
for (uint256 i = 0; i < tokenIds.length; i++) {
_mintToken(msg.sender, tokenIds[i], tokenUris[i]);
}
}
Add an onlyOwner modifier if the desired behaviour is that only the owner can mint tokens for free.
The behaviour of the mintTokenAndTransfer should be exchanging NFTs from the seller with funds from the buyer. However, the seller is receiving funds without transfering any token
The function mintTokenAndTransfer allows the seller to receive funds without the need to transfer his tokens.
The function implement the following loop:
for (uint256 i = 0; i < mintTokenTransfer.tokenIds.length; i++) {
_mintToken(
mintTokenTransfer.seller,
mintTokenTransfer.tokenIds[i],
mintTokenTransfer.tokenUris[i]
);
_distributeRoyality(
mintTokenTransfer.seller,
mintTokenTransfer.price,
mintTokenTransfer.adminCommission,
mintTokenTransfer.tokenIds[i],
mintTokenTransfer.adminWallet
);
_safeTransfer(
mintTokenTransfer.seller,
msg.sender,
mintTokenTransfer.tokenIds[i],
""
);
}
As it can be seen, the first executed function is used to mint tokens directly to the seller, later on, the same minted tokens are transfered from the seller to the buyer, in this case msg.sender. This means that the transfered tokens are previously free minted to the seller which actually is not providing any value to the buyer but he is receiving funds when _distributeRoyality is executed.
Change the implementation of the function to execute itself as the described behaviour.
The function mintTokenAndTransfer is used to mint tokens to the user and distribute the paid funds. The problem is that the tokens are minted to the user before the funds are paid and as the function does not implement a nonReentrant modifier it is possible that an attacker can execute a reentrancy attack.
As the contract is already defined as ReentrancyGuard, add the nonReentrant modifier to the functions to avoid reentrancy attacks.
The contract employs a low-level .transfer method to send ETH, but there are certain drawbacks when the recipient is a smart contract. Specifically, the transfer will fail under the following conditions:
-
The withdrawer smart contract does not have a payable fallback or receive function implemented.
-
The smart contract's payable fallback function consumes more than 2300 gas, which can occur when using a proxy that increases gas usage during the call.
It is worth noting that the sendValue function in the Address library of OpenZeppelin Contract can be utilized to transfer Ether without the restriction of 2300 gas units. To mitigate the risks of reentrancy that may arise from using this function, it is important to closely adhere to the "Check-effects-interactions" pattern and employ OpenZeppelin Contract's ReentrancyGuard contract. For additional information on why the use of Solidity's transfer is no longer recommended, you can refer to the following articles:
If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.
Source Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.
A comprehensive evaluation of smart contract security can never guarantee the absolute absence of vulnerabilities. Such an evaluation is subject to limitations in terms of time, resources, and expertise, and aims to identify as many potential vulnerabilities as possible. Therefore, even after the evaluation, it cannot be ensured that 100% security has been achieved, or that any potential issues with the smart contracts have been identified. It is strongly recommended to conduct subsequent security reviews, bug bounty programs, and on-chain monitoring to ensure continued security.