6.1 Name and Symbol for VeggiaERC721 are not set
VeggiaERC721 is implemented behind proxy contract. All the interactions of the functionality in VeggiaERC721 are routed through the proxy contract. As a result, the storage used by the protocol is that of a proxy contract.
In the constructor, the name and symbol for the contract as passed as part of the inheritance hierarchy. The constructor is only initializing the storage of VeggiaERC721 contract, which is a logic contract.
constructor(address _feeReceiver, string memory _baseUri) ERC721("Veggia", "VEGGIA") Ownable(msg.sender) {
baseURI = _baseUri;
feeReceiver = _feeReceiver;
}
The actual storage is owned by proxy and hence name and symbol should be initialized in the initialize function. However, the latter does not initialise the name and the symbol:
function initialize(address _owner, address _feeReceiver, address _capsSigner, string memory _baseUri) external {
/// @dev Skips owner verification as the proxy is already ownable.
/// @dev Skips initialization check as the proxy handles the initialization check internally.
_transferOwnership(_owner);
baseURI = _baseUri;
capsSigner = _capsSigner;
feeReceiver = _feeReceiver;
// Must be a multiple of 3
freeMintLimit = 6;
freeMintCooldown = 12 hours;
// Prices
capsPriceByQuantity[3] = 0.0003 ether;
capsPriceByQuantity[9] = 0.0006 ether;
capsPriceByQuantity[30] = 0.0018 ether;
premiumCapsPriceByQuantity[3] = 0.0009 ether;
premiumCapsPriceByQuantity[9] = 0.00225 ether;
premiumCapsPriceByQuantity[30] = 0.0054 ether;
premiumPackPrice = 0.0036 ether;
// Set the default royalty to 0
_setDefaultRoyalty(feeReceiver, 0);
}
Moreover, ERC721 does not provide an option to update these variables through set functions.
As a result, the name and symbol are never initialised for VeggiaERC721
Recommendation
Use upgradeable contracts that are designed for proxy patterns.
6.2 VeggiaERC721 is vulnerable to signature replay attacks
The VeggiaERC721 contract supports offchain signatures for minting tokens. The mint3WithSignature function enables users to mint tokens using off-chain signature. However, the current implementation is vulnerable to replay attack if VeggiaERC721 is deployed on multiple chains.
function mint3WithSignature(bytes memory signature, bytes calldata message) external {
// Verify the signature
bytes32 messageHash = keccak256(message);
address recoveredSigner = ECDSA.recover(messageHash, signature);
if (recoveredSigner != capsSigner) revert INVALID_SIGNATURE();
// Decode the message
(address to, uint256 index, bool isPremium) = abi.decode(message, (address, uint256, bool));
// Check if the signature has already been used
if (signatureMintsOf[to][index]) revert SIGNATURE_REUSED();
// Check if the sender is the expected one
/// @dev Only the "to" address can use the signature
if (to != msg.sender) revert INVALID_SENDER(msg.sender, to);
// Mark the signature as used
signatureMintsOf[to][index] = true;
// Mint the NFTs
_open3CapsForSender(isPremium);
emit MintedWithSignature(msg.sender, message, signature);
}
The current implementation provides a guard when VeggiaERC721 is deployed on a single chain. But, in case the contracts are deployed on multiple chains, using the same signature, the tokens can be minted on multiple chains.
Recommendation
Extend VeggiaERC721 from EIP712 which will ensure signatures cannot be replayed across chains. In order to be effective cross-chain, the correct domain initialization for VeggiaERC721 should be implemented.
6.3 Tokens minted by smart contract could be stuck
Minting ERC721 tokens from a smart contract has a risk of tokens being stuck, in case the receiving contract does not have the ability to handle ERC721 tokens. Tokens locked in such contracts are not retrievable.
In order to prevent this, the recommended approach is to mint using _safeMint function. This function invokes hooks on the receiver to ensure the receiver can handle NFT tokens. It will proceed with the minting only for smart contracts that handle NFT tokens and hence prevent the tokens from being locked.
function _open3CapsForSender(bool isPremium) private {
//...
uint256 _tokenId = tokenId;
_mint(msg.sender, _tokenId);
_mint(msg.sender, _tokenId + 1);
_mint(msg.sender, _tokenId + 2);
//...
}
Note: While _safeMint will prevent locking of the ERC721 token in a smart contract, it invokes a hook onERC721Received which can expose to reentrancy. So, when switching to _safeMint reentrancy guard should be applied.
Recommendation
Consider using _safeMint function with reentrancy guard.
6.4 Storage collision in the Proxy contract
In proxy-based contract architectures, maintaining a correct storage layout is crucial for ensuring proper contract functionality. When using a sequential storage layout, storage slots are assigned in a rigid and incremental manner, which can lead to unintended storage overwrites between the proxy and the implementation contract.
In the VeggiaERC721Proxy contract, the initialized state variable is stored in slot 0:
contract VeggiaERC721Proxy is TransparentUpgradeableProxy {
/// @dev The initialization status of the contract.
bool initialized;
// --SNIP
}
Meanwhile, the implementation contract, VeggiaERC721, follows a sequential storage layout and inherits from ERC721, where the first storage slot is used for storing the NFT's name:
contract VeggiaERC721 is ERC721, ERC721Burnable, ERC721TransferLock, ERC721Royalty, Ownable {}
abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Errors {
using Strings for uint256;
string private _name;
// --SNIP
}
Due to this layout mismatch, when the VeggiaERC721 contract initializes the _name variable, it writes to storage slot 0, which is already occupied by the initialized variable in the proxy contract. Consequently, this results in a direct override of the proxy contract's initialized state.
Fortunately, since the _name variable's byte representation will always be greater than 0x0, the initialized flag will not be unintentionally reset to false, preventing a critical oversight where the contract can be reinitialized. However, storage collisions like this are serious concerns as they may lead to unexpected behaviors in future upgrades. Moreover, if new state variables are introduced in the future, they may cause additional conflicts unless proper storage gaps are allocated in advance.
Recommendation
To prevent storage collisions, it is strongly recommended to use OpenZeppelin’s upgradable contract libraries. These libraries manage storage layouts explicitly and ensure that new storage variables can be safely added without causing conflicts. Additionally, when designing proxy-based contracts, explicitly reserving storage gaps (__gap patterns) can help accommodate future variables
6.5 Users can get immediately all the free mint tokens without waiting for post contract deployment
In VeggiaERC721 contract, the freeMint3 function allows the user to mint free tokens up to a limit. After they are minted, there is a cool down period which they have to wait before the next mint.
However, the current implementation of freeMint3 function allows the users to claim their first 6 caps without any wait post-contract deployment.
function freeMint3() external {
uint256 _freeMintLimit = freeMintLimit;
uint256 _freeMintCooldown = freeMintCooldown;
// Time elapsed since the last mint
==> uint256 elapsedTime = block.timestamp - lastMintTimestamp[msg.sender];
uint256 intervals = elapsedTime / _freeMintCooldown;
// Total accumulated caps balance
uint256 totalCaps = intervals * 3;
if (totalCaps > _freeMintLimit) {
totalCaps = _freeMintLimit;
}
//...
}
The logic calculates the elapsed time as the difference between the current time and lastMintTimestamp of the users. As the default value for any account in lastMintTimestamp will be zero, every user will be able to claim their first set of free tokens without any wait for post-contract deployment.
Recommendation
One possible recommendation is to introduce a variable that tracks the time of the contract's deployment (set in the initialize function). During the first user's mint, consider calculating the elapsed time using the contract's deployment timestamp instead of lastMintTimestamp:
uint256 elapsedTime = lastMintTimestamp[msg.sender] == 0 ?
block.timestamp - deployedTimestamp :
block.timestamp - lastMintTimestamp[msg.sender];
Resolution
It is a design choice to let the user mint tokens immediately the first time.
6.6 Missing validation in setFreeMintCooldown function could lead to revert
In setFreeMintCooldown function, there is no validation for an acceptable range of values for cool down time to unlock a new freeMint3. As a result, if it was set to 0, it will cause a revert during freeMint3 function call.
function freeMint3() external {
//...
uint256 _freeMintCooldown = freeMintCooldown;
//...
uint256 intervals = elapsedTime / _freeMintCooldown;
//...
}
Likewise, many of the owner functions do not validate input values. Below are a few examples of such functions.
function setFreeMintCooldown(uint256 cooldown) external onlyOwner {
freeMintCooldown = cooldown; // can be set to 0
}
function setFeeReceiver(address receiver) external onlyOwner {
feeReceiver = receiver; // can be set to address(0)
}
function setCapsSigner(address _capsSigner) external onlyOwner {
capsSigner = _capsSigner; // can be set to address(0)
}
Recommendation
Add validation that restricts acceptable range of values for freeMintCooldown state variable. Also, add validations to owner functions.
6.7 Caps purchased with premiumPackPrice is inconsistent with comments in the contract
The caps purchased with premiumPackPrice have two different configurations per the comments in the contract. At the time of declaration of the state variable, the pack contains 1 NFT, 10 caps, and 3 premium caps.
* @notice The price of the premium pack.
* 1 NFT mint + 10 caps + 3 premium caps.
*/
uint256 public premiumPackPrice;
But buyPremiumPack function mints 1 NFT, 12 caps and 3 premium caps.
/**
* @notice Buy a premium pack with the price of {premiumPackPrice}.
* @dev The premium pack contains 1 NFT mint + 12 caps + 3 premium caps.
*/
function buyPremiumPack() external payable {
if (msg.value != premiumPackPrice) revert WRONG_VALUE();
// Give the caps to the buyer (12 caps because 12 is a multiple of 3)
paidCapsBalanceOf[msg.sender] += 12;
// Give the premium caps to the buyer
paidPremiumCapsBalanceOf[msg.sender] += 3;
// Mint the NFT in the pack
uint256 _tokenId = tokenId; // Cache the tokenId in memory
_mint(msg.sender, _tokenId);
//....
}
Recommendation
Update the natspec to reflect the actual caps minted for a user in a premium pack.
6.8 CapsPrice should be configurable only for multiples of 3
Since the buyCaps function only allows buying caps in multiples of 3, the configurable prices should also be restricted to multiples of 3.
Currently, there is no validation in the set functions:
function setCapsPrice(uint256 quantity, uint256 price) external onlyOwner {
capsPriceByQuantity[quantity] = price;
emit CapsPriceChanged(quantity, price);
}
function setPremiumCapsPrice(uint256 quantity, uint256 price) external onlyOwner {
premiumCapsPriceByQuantity[quantity] = price;
emit PremiumCapsPriceChanged(quantity, price);
}
So, the owner can potentially configure any quantity, but the user can only buy in multiples of 3.
function buyCaps(bool isPremium, uint256 quantity) external payable {
if (quantity % 3 != 0) revert WRONG_CAPS_QUANTITY();
//...
}
Recommendation
Implement validation similar to the buyCaps function in the setCapsPrice and setPremiumCapsPrice functions.
6.9 Ownership is more secure with two-step transfers
As the owner is a critical role in managing admin functions of the protocol, it is recommended to implement a two-step transfer process. In the first step, the current owner initiates the process for the new account to take over ownership. As a second step, the new account will claim ownership. This ensures that the caller of the claiming function has control of the new account.
contract VeggiaERC721 is ERC721, ERC721Burnable, ERC721TransferLock, ERC721Royalty, Ownable {
Recommendation
Consider using Ownable2Step library contract of openzeppelin instead of Ownable.
