steemhunt / dixel-v2-contract Goto Github PK
View Code? Open in Web Editor NEWLicense: BSD 3-Clause "New" or "Revised" License
License: BSD 3-Clause "New" or "Revised" License
Hello,
How do you withdraw NFT sales from this contract?
If minting has already begun, the owner or any user can't mint the NFT. The code below ensures this.
However, the owner can bypass this because updateMetadata()
allows the owner to change the minting starts date when mintingBeginsFrom
equals block.timestamp
.
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 255 to 257 in 6495dd6
To exploit the above issue, the owner creates a new NFT with a large enough mintingBeginsFrom
in advance. And then, he will craft the below transactions in order. (All transactions in the block have the same block.timestamp
)
mintingBeginsFrom
to block.timestamp
.mintingBeginsFrom
to large enough mintingBeginsFrom
again.The minting starts date can be changed even after minting.
updateMetadata()
should revert when _metaData.mintingBeginsFrom
equals block.timestamp
.
Example patch
function updateMetadata(bool whitelistOnly, bool hidden, uint24 royaltyFriction, uint40 mintingBeginsFrom, uint152 mintingCost) external onlyOwner {
if(royaltyFriction > MAX_ROYALTY_FRACTION) revert DixelClubV2__InvalidRoyalty(royaltyFriction);
- if(_metaData.mintingBeginsFrom != mintingBeginsFrom && uint40(block.timestamp) > _metaData.mintingBeginsFrom) revert DixelClubV2__AlreadyStarted();
+ if(_metaData.mintingBeginsFrom != mintingBeginsFrom && uint40(block.timestamp) >= _metaData.mintingBeginsFrom) revert DixelClubV2__AlreadyStarted();
There is a mint event on the mint
function, but there isn't on the burn
function.
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 115 to 127 in 657250a
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 108 to 113 in 657250a
The operator could not track the on-chain events easilty.
Add a Burn event and emit it as same as the mint function for better handling on the off-chain side.
cc/ @Jinmo
Hi, can't _initializedAt
at
bool
instead?
I saw it's used very little, and I don't get clearly its function. Can you clarify it?
On createCollection() method and updateDescription() method, they filter double quote character (0x22) to make it impossible to return invalid JSON string on tokenJSON() method and contractJSON() method.
function createCollection(
string memory name,
string memory symbol,
Shared.MetaData memory metaData,
uint24[PALETTE_SIZE] memory palette,
uint8[TOTAL_PIXEL_COUNT] memory pixels
) external payable returns (address payable) {
...
// Validate `symbol`, `name` and `description` to ensure generateJSON() creates a valid JSON
require(!StringUtils.contains(name, 0x22), "NAME_CONTAINS_MALICIOUS_CHARACTER");
require(!StringUtils.contains(symbol, 0x22), "SYMBOL_CONTAINS_MALICIOUS_CHARACTER");
require(!StringUtils.contains(metaData.description, 0x22), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
...
}
function updateDescription(string memory description) external onlyOwner {
require(bytes(description).length <= 1000, "DESCRIPTION_TOO_LONG"); // ~900 gas per character
require(!StringUtils.contains(description, 0x22), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
_metaData.description = description;
}
function tokenJSON(uint256 tokenId) public view checkTokenExists(tokenId) returns (string memory) {
return string(abi.encodePacked(
'{"name":"',
_symbol, ' #', ColorUtils.uint2str(tokenId),
'","description":"',
_metaData.description,
'","external_url":"https://dixel.club/collection/',
ColorUtils.uint2str(block.chainid), '/', StringUtils.address2str(address(this)), '/', ColorUtils.uint2str(tokenId),
'","image":"',
generateBase64SVG(tokenId),
'"}'
));
}
function contractJSON() public view returns (string memory) {
return string(abi.encodePacked(
'{"name":"',
_name,
'","description":"',
_metaData.description,
'","image":"',
generateBase64SVG(0),
'","external_link":"https://dixel.club/collection/',
ColorUtils.uint2str(block.chainid), '/', StringUtils.address2str(address(this)),
'","seller_fee_basis_points":"',
ColorUtils.uint2str(_metaData.royaltyFriction),
'","fee_recipient":"',
StringUtils.address2str(owner()),
'"}'
));
}
However, backslash character (0x5c) can also be used to create invalid JSON string by escaping existing double quote character.
For example, when _metadata.description is A\
([0x41, 0x5c]), tokenJSON() method would return like:
{"name":"symbol#0","description":"A\","external_url":"https://dixel.club/collection/0/0x0000000000000000000000000000000000000000/0","image":"data:image/svg+xml;base64,"}
which is not a valid JSON string.
Therefore, I suggest adding the validation code to filter backslash character (0x5c) like as below.
function createCollection(
string memory name,
string memory symbol,
Shared.MetaData memory metaData,
uint24[PALETTE_SIZE] memory palette,
uint8[TOTAL_PIXEL_COUNT] memory pixels
) external payable returns (address payable) {
...
// Validate `symbol`, `name` and `description` to ensure generateJSON() creates a valid JSON
require(!StringUtils.contains(name, 0x22) && !StringUtils.contains(name, 0x5c), "NAME_CONTAINS_MALICIOUS_CHARACTER");
require(!StringUtils.contains(symbol, 0x22) && !StringUtils.contains(symbol, 0x5c), "SYMBOL_CONTAINS_MALICIOUS_CHARACTER");
require(!StringUtils.contains(metaData.description, 0x22) && !StringUtils.contains(metaData.description, 0x5c), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
...
}
function updateDescription(string memory description) external onlyOwner {
require(bytes(description).length <= 1000, "DESCRIPTION_TOO_LONG"); // ~900 gas per character
require(!StringUtils.contains(description, 0x22) && !StringUtils.contains(description, 0x5c), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
_metaData.description = description;
}
Without the Ownable contract, it is impossible to set essential options such as setting the 2nd fee in Opensea.
This is an important contract that must be implemented.
test
Ref. : https://medium.com/@ItsCuzzo/using-merkle-trees-for-nft-whitelists-523b58ada3f9
Don't use for loop for whitelisting.
It is not efficient, and when the number of whilelist increases, the for loop must be reverted.
dixel-v2-contract/contracts/DixelClubV2Factory.sol
Lines 74 to 75 in 657250a
Factory calls init
when the user creates the collection.
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 51 to 73 in 657250a
init
function transfers its ownership to the owner which is the first argument of the function (msg.sender of the factory).
There is missing check of the owner should not be the contract.
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 100 to 103 in 657250a
If the owner is a contract, the owner can control the possibility that user can mint or can not mint by reverting on the recieve
function.
The owner can initialize maxSupply
only once which limits the maximum NFT counts.
Nevertheless the maxSupply
sets properly, reverting the owner contract allows the owner can control the miting possibility whenever the owner want.
There is a _transferOwnership
function which has an opportutnity to transfer the ownership to the contract address. So the design should be changed to the owner claims the mintingCost instead of sending the mintingCost to the owner diretly. Also, it could reduce the gas when the user mints the NFT.
cc/ @Jinmo
dixel-v2-contract/contracts/DixelClubV2Factory.sol
Lines 47 to 63 in 74f4095
metaData.mintingBeginsFrom
. It allow an attacker to create NFT contract with an invalid minting starts date.
Attackers can create the NFT contract as if minting had started in the past. (e.g. Setting metaData.mintingBeginsFrom
to zero means minting had started on the 1970-01-01)
In createCollection()
function, If the metaData.mintingBeginsFrom
less than block.timestamp
, it should be replaced with block.timestamp
.
Example patch
function createCollection(
string memory name,
string memory symbol,
Shared.MetaData memory metaData,
uint24[PALETTE_SIZE] memory palette,
uint8[TOTAL_PIXEL_COUNT] memory pixels
) external payable returns (address payable) {
+ // Neutralize minting starts date.
+ if (metaData.mintingBeginsFrom < block.timestamp)
+ metaData.mintingBeginsFrom = block.timestamp
address payable nftAddress = _createClone(nftImplementation);
DixelClubV2NFT newNFT = DixelClubV2NFT(nftAddress);
newNFT.init(msg.sender, name, symbol, metaData, palette, pixels);
AS-IS)
function updateBeneficiary(address newAddress, uint256 newCreationFee, uint256 newMintingFee) external onlyOwner {
if(newAddress == address(0)) revert DixelClubV2Factory__ZeroAddress();
if(newMintingFee > FRICTION_BASE) revert DixelClubV2Factory__InvalidFee();
beneficiary = newAddress;
mintingFee = newMintingFee;
creationFee = newCreationFee;
}
if exist case that update only one of the beneficiary or fees, how about split each parameters to functions?
It seems that human error can be reduced.
TO-BE)
function updateBeneficiary(address newAddress) external onlyOwner {
if(newAddress == address(0)) revert DixelClubV2Factory__ZeroAddress();
beneficiary = newAddress;
}
function updateMintingFee(uint256 newMintingFee) external onlyOwner {
if(newMintingFee > FRICTION_BASE) revert DixelClubV2Factory__InvalidFee();
mintingFee = newMintingFee;
}
function updateCreationFee(uint256 newCreationFee) external onlyOwner {
creationFee = newCreationFee;
}
The smallest uint type in solidity is uint8, and at the moment this is what Dixel2 uses to save each pixel's value.
This gives us 256 values to save in each uint8; but we need only 16, as the pallete size is 16.
This means that by using bit operations, we can utilize each uint8 to save 2 pixels.
For example, pixels[0] would be the colors of both the first and second pixel. The rightmost 4 bits would be the first pixel, and the leftmost 4 bits would be the second pixel.
While this will add some complexity to the front end and back end in creating/reading the pixels array, this saves 360,000 gas upon collection creation - a significant amount. (It is up to you to decide whether it is worth it.)
Basically, the formulas for getting the pixel values at _generateSVG
(the only place that reads the values) will be:
uint256 prev = pixels[y * CANVAS_SIZE / 2] & 15;
uint256 current = (pixels[y * CANVAS_SIZE/2 + x/2] >> 4*(x%2)) & 15;
In a few hours I will try to create a patch/PR.
AS-IS
function getWhitelistAllowanceLeft(address wallet) external view returns (uint256 allowance) {
uint256 length = _whitelist.length; // gas saving
for (uint256 i; i != length;) {
if (_whitelist[i] == wallet) {
allowance++;
}
unchecked {
++i;
}
}
return allowance;
}
function getWhitelistIndex(address wallet) external view returns (uint256) {
uint256 length = _whitelist.length; // gas saving
for (uint256 i; i != length;) {
if (_whitelist[i] == wallet) {
return i;
}
unchecked {
++i;
}
}
revert DixelClubV2__NotWhitelisted();
}
That clone _whitelist from storage to use in loop is expected the effect of reducing SLOAD calls.
First, this is the test result.
address[20] addrs = [ ... ]
function origin() external {
uint256 dummy = 0;
uint256 len = addrs.length;
for (uint256 i; i < len;) {
if (addrs[i] == address(0)) {
dummy += i;
}
unchecked {
i++;
}
}
}
function suggest1() external {
uint256 dummy = 0;
address[] memory temp = addrs;
uint256 len = temp.length;
for (uint256 i; i < len;) {
if (temp[i] == address(0)) {
dummy += i;
}
unchecked {
i++;
}
}
}
function suggest2() external {
uint256 dummy = 0;
address[] storage temp = addrs;
uint256 len = temp.length;
for (uint256 i; i < len;) {
if (temp[i] == address(0)) {
dummy += i;
}
unchecked {
i++;
}
}
}
origin() gasUsed : 57873
suggest1() gasUsed : 43636
suggest2() gasUsed : 57925
suggest1() what clone storage data to memory variable seems to be the best case.
TO-BE
function getWhitelistAllowanceLeft(address wallet) external view returns (uint256 allowance) {
// gas saving
address[] memory clone = _whitelist;
uint256 length = clone.length;
for (uint256 i; i != length;) {
if (clone[i] == wallet) {
allowance++;
}
unchecked {
++i;
}
}
return allowance;
}
function getWhitelistIndex(address wallet) external view returns (uint256) {
// gas saving
address[] memory clone = _whitelist;
uint256 length = clone.length;
for (uint256 i; i != length;) {
if (clone[i] == wallet) {
return i;
}
unchecked {
++i;
}
}
revert DixelClubV2__NotWhitelisted();
}
dixel-v2-contract/contracts/DixelClubV2NFT.sol
Lines 119 to 124 in 657250a
Line 119: Mint the NFT, and then call the _safeMint
function.
Line 124: Increment the tokenId
dixel-v2-contract/contracts/lib/ERC721Initializable.sol
Lines 250 to 260 in 657250a
_safeMint
calls _checkOnERC721Received
which has a re-entrancy opportunity.
When an attacker has an opportunity to re-enter the NFT contract, the attacker can emit the Mint
events twice with the same tokenId, by following steps:
onERC721Received
function of the attacker's contract. (tokenID = X)_safeMint
function returns, so attacker can mint twice with the same tokenId.Add a nonReentrant modifier on the Mint/Burn functions. (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol)
cc/ @Jinmo
DixelClubV2NFT has an ether receive function:
receive() external payable {}
It is not necessary for the functioning of the contract - as ether should only be sent during a mint
call.
The receive
function will enable sending ether to the contract outside of the mint
function.
This ether will be stuck in the contract, as there's no way to pull ether.
While this is unlikely to happen, this means that if somebody accidently sends ether (/native token) to the contract without calling mint
, his funds would be lost.
Since the contract does not need this method, and it might cause harm, I suggest removing this receive
function.
contract DixelClubV2NFT is ERC721Enumerable, Ownable, SVGGenerator {
...
address[] private _whitelist;
...
}
The whitelist of the DixelClubV2NFT contract is implemented as an Array.
Each function uses the following computational cost.
How about using mapping to save gas cost?
mapping(addres => bool) public whitelist;
Please let me know if there's anything I didn't think of.
Community Audit for Dixel V2 contract is now over.
In total, 13 developers have contributed in this review process.
@contract-whale
@Nipol
@assafom
@syl-doo2
@dbadoy
@keon
@sirius651
@jonghwaC
@n1net4il
@kjsman
@junomonster
@kirasys
@inmarelibero
Thank you guys all for your valuable time and contributions ๐๐ป
I'm going to evaluate all contributions as fair as possible considering the importance of the contributions and amount of work put into the reviewing process.
While I'm working on the distribution details, please leave your wallet address here for receiving your reward (USDT wallet, Please specify the network: Ethereum, BNB Smart Chain, Solana, AVAX C-Chain, Polygon, Tron network).
If you don't want to put your address here publicly, you can send it to our email instead: [email protected]
I just had a cool idea to keep the whitelist as array, but save lots of gas ๐
You can keep the current mint method if you want, but also add another one, which will take an additional parameter - the sender's position in the whitelist array.
It will then just verify that _whitelist[parameter] == msg.sender
, and then delete that value.
This way it does not have to traverse the array at all! So obviously it's a huge difference.
How it will work is that the front end can search for an address's position in the whitelist array (free view function), and then pass that as parameter to the new mint function.
I think this is probably clear enough, let me know of any questions.
The SVGGenerator generates images which are 25 pixels wide. You can see it for example in test-svg.svg - we have the following SVG path:
M0 1h25
This tells the SVG to draw 25 pixels - but the size of the canvas is only 24 pixels.
I believe the culprit is this line, which is executed on the last pixel of the line:
paths[current] = string(abi.encodePacked(paths[current], "h", ColorUtils.uint2str(width + 1)));
If we change it to draw only width
instead of width + 1
, the resulting canvas would be 24 pixels wide, as it should.
At the moment it is drawing the last pixel of each row twice.
To see why - let's say the last pixel has changed in color. In the current implementation, this is the code:
if (prev == current) {
width++;
} else {
paths[prev] = string(abi.encodePacked(paths[prev], "h", ColorUtils.uint2str(width)));
width = 1;
paths[current] = string(abi.encodePacked(paths[current], "M", ColorUtils.uint2str(x), " ", ColorUtils.uint2str(y)));
}
if (x == CANVAS_SIZE - 1) {
paths[current] = string(abi.encodePacked(paths[current], "h", ColorUtils.uint2str(width + 1)));
}
So the "else" block would set the width to 1, and then the if (x == CANVAS_SIZE - 1)
block would increment the width to 2 - but the real width should be only 1.
This is why the width should be only width
and not width + 1
.
It is now my bed time, tomorrow if I'll have time I'll try adding some tests for this ๐
Note: although there is another discussion on whether to change the implementation to mapping, I'm posting this issue in case you decide to stay with the array.
At the moment, when minting, this is the whitelist process:
// For whitelist only collections
if (_metaData.whitelistOnly) {
require(isWhitelistWallet(msg.sender), "NOT_IN_WTHIELIST");
_removeWhitelist(msg.sender); // decrease allowance by 1
}
This means the contract will iterate once over the whitelist array in isWhitelistWallet
, and then iterate again on _removeWhitelist
.
While the second iteration will be cheaper, it will still be 100 gas for every address that is read from the whitelist. So address #200 will have to pay unnecessary 20000 gas.
Instead of doing this, you can skip calling isWhitelistWallet
, and just call _removeWhitelist
, and change it so it will revert if the address it not found.
So you can change the _removeWhitelist
from this:
function _removeWhitelist(address wallet) private {
for (uint256 i = 0; i < _whitelist.length; i++) {
if (_whitelist[i] == wallet) {
_whitelist[i] = _whitelist[_whitelist.length - 1]; // put the last element into the delete index
_whitelist.pop(); // delete the last element to decrease array length;
break; // delete the first matching one and stop
}
}
}
To this:
function _removeWhitelist(address wallet) private {
for (uint256 i = 0; i < _whitelist.length; i++) {
if (_whitelist[i] == wallet) {
_whitelist[i] = _whitelist[_whitelist.length - 1]; // put the last element into the delete index
_whitelist.pop(); // delete the last element to decrease array length;
return; // delete the first matching one and stop
}
}
require(false, "NOT_IN_WHITELIST")
}
And change the minting whitelist block to this:
// For whitelist only collections
if (_metaData.whitelistOnly) {
_removeWhitelist(msg.sender); // decrease allowance by 1, revert if not in whitelist
}
And this will save you O(n) of iteration on the list.
Note: removeWhitelist
also uses _removeWhitelist
, so make sure you're ok with it reverting if the owner supplies an address which is not whitelisted. If not, just change removeWhitelist
implementation to not revert if not found... You probably get the picture.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.