Usage of for loops might lead to large amounts of tokens not beeing transferable.
Juglipaff opened this issue · 8 comments
Usage of for loops might lead to large amounts of tokens not beeing transferable.
In the code bellow, if the variables tokens_to_burn or tokens_to_mint are big enough, that would lead to an Out of Gas error and prevent large token transfers.
Lines 324 to 340 in 14c1362
Maybe it is possible to use some clever bit manipulation to do this kind of minting/burning in sub-O(n) time? Kinda like ERC721A does it
Also, since tokens_to_burn and tokens_to_mint both equal amount / unit, wouldn't it be more gas efficient to do a single for loop in which you transfer ERC721s from from to to, instead of doing two separate ones?
A NFT mint is super gas heavy anyway. And there very low chance you run it this issue unless one hang one self with deploying a token with 1 for the mint unit and minting millions or billions of tokens. Why would want to have over 100k NFT the first hour-day? That would be crazy and dumb and would be broken second it was deployed.
A NFT mint is super gas heavy anyway. And there very low chance you run it this issue unless one hang one self with deploying a token with 1 for the mint unit and minting millions or billions of tokens. Why would want to have over 100k NFT the first hour-day? That would be crazy and dumb and would be broken second it was deployed.
It would revert not with millions or billions but in the hundreds range. That can happen with whales, during pool migrations on DEXes etc. even with the standard 10k total supply.
If this wants to become an official EIP it should support all possible usage cases, including those that might seems dumb at first.
BTW, batch mints with ERC721A are cheap. Source: https://www.alchemy.com/blog/erc721-vs-erc721a-batch-minting-nfts
As a proof of my words, here is the test contract:
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
import "./ERC404.sol";
contract TestTransferERC404 is ERC404 {
constructor(uint256 amount) ERC404("TestTransfer", "TT", 18, amount, msg.sender) {
balanceOf[msg.sender] = totalSupply;
uint256 tokens_to_mint = totalSupply / _getUnit();
for (uint256 i; i < tokens_to_mint; ++i) {
_mint(msg.sender);
}
}
function tokenURI(uint256 /*id*/) public pure override returns (string memory) {
return "";
}
}
contract TestTransfer {
TestTransferERC404 public immutable erc404;
constructor(uint256 amount) {
erc404 = new TestTransferERC404(amount);
erc404.approve(address(this), type(uint256).max);
}
function transfer() external {
erc404.transferFrom(address(this), address(1), erc404.balanceOf(address(this)));
}
}
To test:
- Deploy TestTransfer with amount == 350. In this transaction, we mint 350 TestTransferERC404 to TestTransfer contract.
- Call transfer() to transfer all tokens in contract's balance to address(1)
Result: transfer() is unable to be called because it exceedes block gas limit.
There is easy fix without implementing something like ERC721A or ERC721Psi. Start the initial price higher. Then there is no possibility that to happen then.
Let say you want to migrate liquidity, set up a time based pause of minting for migration in the contract you implement this one with. Pretty sure it one done on purpose like this, if not it has become feature of it.
Sometimes optimizing is not useful. In this case there no reason for it to start low in price anyway vs the unit of mint since it just unit bias playing with you, also increasing the unit and supply you can price it right so it does not break.
One can easily use common standards and remake this abstract contract to ones liking. ERC404 is a meme contract, it is a play on http 404 error, not found. It just mocking the 3 lettter standards and improvement proposals.
If this was any serious effort they should have based it on the concept around ERC-1046 or inherited it and built with it and submitted an EIP. It is the ordinal of the evm smart contract world, quick and dirty with high cost.
This is very good point, the reason why we have 320 minting limit of the token. We need a gas optimized version of ERC-404
There is easy fix without implementing something like ERC721A or ERC721Psi. Start the initial price higher. Then there is no possibility that to happen then.
Project developers can't just set a high initial price of the token. If this was this simple, everyone would be millionaires. Starting with a high initial price requires tokens to either have stellar marketing, or have high liquidity to back it up, which can't be the case for the most projects.
Also, tokens are used not only for speculative or economic purposes. Some projects use them for their internal architecture. So setting a price for them sometimes does not even make sense.
Let say you want to migrate liquidity, set up a time based pause of minting for migration in the contract you implement this one with. Pretty sure it one done on purpose like this, if not it has become feature of it.
No project will make logic like this just for one token that does not follow standards. It takes too much effort to develop while providing minimal benefits. That's why standards exists and why tokens should follow them.
One can easily use common standards and remake this abstract contract to ones liking. ERC404 is a meme contract, it is a play on http 404 error, not found. It just mocking the 3 lettter standards and improvement proposals.
I did not know this was a meme contract. To me it seemed like an actual serious effort, albeit very confusing with the naming.
If this was any serious effort they should have based it on the concept around ERC-1046 or inherited it and built with it and submitted an EIP. It is the ordinal of the evm smart contract world, quick and dirty with high cost.
That does not mean that this issue should not be fixed and just be forgotten. Every auditing company will mark this issue as having at least medium severity, so some effort should be done in addressing it.