Platform: Code4rena
Start Date: 13/12/2023
Pot Size: $36,500 USDC
Total HM: 18
Participants: 110
Period: 8 days
Judge: 0xTheC0der
Id: 311
League: ETH
Rank: 75/110
Findings: 2
Award: $28.20
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xmystery
Also found by: 00xSEV, 0xCiphky, 0xDING99YA, 0xhitman, ABAIKUNANBAEV, Aamir, BARW, IllIllI, King_, MrPotatoMagic, Pechenite, SovaSlava, SpicyMeatball, Topmark, Ward, ZanyBonzy, ast3ros, bart1e, cheatc0d3, deth, developerjordy, hals, imare, kaveyjoe, ktg, leegh, osmanozdemir1, peanuts, roland, rvierdiiev, shaka, sivanesh_808, spacelord47
7.359 USDC - $7.36
Note: I've removed the specific instances from the bot race and 4naly3er
| Issue | Instances | |
|---|---|---|
| [M‑01] | _safeMint() should be used rather than _mint() wherever possible | 1 |
Total: 1 instances over 1 issues
| Issue | Instances | |
|---|---|---|
| [L‑01] | payable function does not transfer Eth | 2 |
| [L‑02] | Code does not follow the best practice of check-effects-interaction | 34 |
| [L‑03] | Input array lengths may differ | 3 |
| [L‑04] | Upgradeable contract not initialized | 7 |
Total: 46 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | constants should be defined rather than using magic numbers | 6 |
| [N‑02] | public functions not called by the contract should be declared external instead | 10 |
| [N‑03] | Common functions should be refactored to a common base contract | 1 |
| [N‑04] | Consider making contracts Upgradeable | 1 |
| [N‑05] | Consider using a struct rather than having many function input parameters | 2 |
| [N‑06] | Consider using descriptive constants when passing zero as a function argument | 9 |
| [N‑07] | Consider using named function arguments | 16 |
| [N‑08] | Consider using named returns | 38 |
| [N‑09] | Custom errors should be used rather than revert()/require() | 4 |
| [N‑10] | Function state mutability can be restricted to pure | 2 |
| [N‑11] | Function state mutability can be restricted to view | 2 |
| [N‑12] | Large multiples of ten should use scientific notation | 2 |
| [N‑13] | Missing event for critical parameter change | 3 |
| [N‑14] | NatSpec: Error declarations should have @dev tags | 2 |
| [N‑15] | NatSpec: Error declarations should have @notice tags | 2 |
| [N‑16] | NatSpec: Function @return tag is missing | 12 |
| [N‑17] | NatSpec: Non-public state variable declarations should use @dev tags | 5 |
| [N‑18] | Not using the named return variables anywhere in the function is confusing | 5 |
| [N‑19] | Not using the named return variables anywhere in the function is confusing | 5 |
| [N‑20] | Style guide: Contract does not follow the Solidity style guide's suggested layout ordering | 1 |
| [N‑21] | Style guide: Control structures do not follow the Solidity Style Guide | 27 |
| [N‑22] | Style guide: Extraneous whitespace | 59 |
| [N‑23] | Style guide: Non-external/public variable names should begin with an underscore | 11 |
| [N‑24] | Style guide: Top-level declarations should be separated by at least two lines | 1 |
| [N‑25] | The nonReentrant modifier should occur before all other modifiers | 1 |
| [N‑26] | Use _disableInitializers() in the constructor body, rather than using the initializer modifier | 6 |
| [N‑27] | Use of override is unnecessary | 5 |
| [N‑28] | Variables need not be initialized to zero | 7 |
Total: 245 instances over 28 issues
_safeMint() should be used rather than _mint() wherever possible_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function. In the cases below, _mint() does not call ERC721TokenReceiver.onERC721Received() on the recipient.
There is one instance of this issue:
File: src/VerbsToken.sol 310: _mint(to, verbId);
GitHub: 310
payable function does not transfer EthFunds sent along with the call to this function will be lost. Unless this is a gas-saving optimization, the payable keyword should be removed
There are 2 instances of this issue:
File: src/ERC20TokenEmitter.sol 64 constructor( 65 address _manager, 66 address _protocolRewards, 67 address _protocolFeeRecipient 68: ) payable TokenEmitterRewards(_protocolRewards, _protocolFeeRecipient) initializer {
GitHub: 64
File: src/abstract/TokenEmitter/TokenEmitterRewards.sol 7 constructor( 8 address _protocolRewards, 9 address _revolutionRewardRecipient 10: ) payable RewardSplits(_protocolRewards, _revolutionRewardRecipient) {}
GitHub: 7
Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.
There are 34 instances of this issue:
File: src/CultureIndex.sol /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 225: newPiece.pieceId = pieceId; /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 226 newPiece.totalVotesSupply = _calculateVoteWeight( 227 erc20VotingToken.totalSupply(), 228 erc721VotingToken.totalSupply() 229: ); /// @audit totalSupply() called prior to this assignment in createPiece(), via: createPiece() /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 230: newPiece.totalERC20Supply = erc20VotingToken.totalSupply(); /// @audit totalSupply() called prior to this assignment in createPiece(), via: createPiece() /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 231: newPiece.metadata = metadata; /// @audit totalSupply() called prior to this assignment in createPiece(), via: createPiece() /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 232: newPiece.sponsor = msg.sender; /// @audit totalSupply() called prior to this assignment in createPiece(), via: createPiece() /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 233: newPiece.creationBlock = block.number; /// @audit totalSupply() called prior to this assignment in createPiece(), via: createPiece() /// @audit insert() called prior to this assignment in createPiece(), via: createPiece() 234: newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; /// @audit getPastVotes() called prior to this assignment in _vote(), via: _vote(), _getPastVotes() 316: votes[pieceId][voter] = Vote(voter, weight); /// @audit getPastVotes() called prior to this assignment in _vote(), via: _vote(), _getPastVotes() 317: totalVoteWeights[pieceId] += weight; /// @audit getMax() called prior to this assignment in dropTopVotedPiece(), via: dropTopVotedPiece(), getTopVotedPiece(), topVotedPieceId() /// @audit size() called prior to this assignment in dropTopVotedPiece(), via: dropTopVotedPiece(), getTopVotedPiece(), topVotedPieceId() 526: pieces[piece.pieceId].isDropped = true;
GitHub: 225, 226, 230, 230, 231, 231, 232, 232, 233, 233, 234, 234, 316, 317, 526, 526
File: src/ERC20TokenEmitter.sol /// @audit yToX() called prior to this assignment in buyToken(), via: buyToken(), getTokenQuoteForEther() /// @audit depositRewards() called prior to this assignment in buyToken(), via: buyToken(), _handleRewardsAndGetValueToSend(), _depositPurchaseRewards() 187: emittedTokenWad += totalTokensForBuyers; /// @audit yToX() called prior to this assignment in buyToken(), via: buyToken(), getTokenQuoteForEther() /// @audit depositRewards() called prior to this assignment in buyToken(), via: buyToken(), _handleRewardsAndGetValueToSend(), _depositPurchaseRewards() 188: if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;
File: src/VerbsToken.sol /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 298: newPiece.pieceId = artPiece.pieceId; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 299: newPiece.metadata = artPiece.metadata; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 300: newPiece.isDropped = artPiece.isDropped; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 301: newPiece.sponsor = artPiece.sponsor; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 302: newPiece.totalERC20Supply = artPiece.totalERC20Supply; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 303: newPiece.quorumVotes = artPiece.quorumVotes; /// @audit MAX_NUM_CREATORS() called prior to this assignment in _mintTo(), via: _mintTo() /// @audit getTopVotedPiece() called prior to this assignment in _mintTo(), via: _mintTo() 304: newPiece.totalVotesSupply = artPiece.totalVotesSupply;
GitHub: 298, 298, 299, 299, 300, 300, 301, 301, 302, 302, 303, 303, 304, 304
If the caller makes a copy-paste error, the lengths may be mismatched and an operation believed to have been completed may not in fact have been completed (e.g. if the array being iterated over is shorter than the one being indexed into).
There are 3 instances of this issue:
File: src/CultureIndex.sol /// @audit creatorArray[] 237: newPiece.creators.push(creatorArray[i]); /// @audit creatorArray[] /// @audit creatorArray[] 244: emit PieceCreatorAdded(pieceId, creatorArray[i].creator, msg.sender, creatorArray[i].bps);
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user. Even if the function is currently empty, or its current functionality is accomplished in another way, a future version of OpenZeppelin may introduce new functionality in the function and, without a new security review, things will unexpectedly be vulnerable when fresh contract instances are deployed with the new version.
There are 7 instances of this issue:
File: src/AuctionHouse.sol /// @audit missing __Ownable2Step_init() 39: contract AuctionHouse is
GitHub: 39
File: src/CultureIndex.sol /// @audit missing __Ownable2Step_init() 20: contract CultureIndex is
GitHub: 20
File: src/ERC20TokenEmitter.sol /// @audit missing __Ownable2Step_init() 17: contract ERC20TokenEmitter is
GitHub: 17
File: src/MaxHeap.sol /// @audit missing __Ownable2Step_init() 14: contract MaxHeap is VersionedContract, UUPS, Ownable2StepUpgradeable, ReentrancyGuardUpgradeable {
GitHub: 14
File: src/NontransferableERC20Votes.sol /// @audit missing __ERC20Votes_init() /// @audit missing __Ownable2Step_init() 29: contract NontransferableERC20Votes is Initializable, ERC20VotesUpgradeable, Ownable2StepUpgradeable {
File: src/VerbsToken.sol /// @audit missing __Ownable2Step_init() 33: contract VerbsToken is
GitHub: 33
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 6 instances of this issue:
File: src/AuctionHouse.sol /// @audit 50000 426 assembly { 427 // Transfer ETH to the recipient 428 // Limit the call to 50,000 gas 429 success := call(50000, _to, _amount, 0, 0, 0, 0) 430: }
GitHub: 426
File: src/CultureIndex.sol /// @audit 1e18 285: return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18);
GitHub: 285
File: src/libs/VRGDAC.sol /// @audit 1e18 35: decayConstant = wadLn(1e18 - _priceDecayPercent); /// @audit 1e18 74: wadPow(1e18 - priceDecayPercent, wadDiv(soldDifference, perTimeUnit)) /// @audit 1e18 91: wadPow(1e18 - priceDecayPercent, timeSinceStart - unsafeWadDiv(sold, perTimeUnit)) - /// @audit 1e18 92: wadPow(1e18 - priceDecayPercent, timeSinceStart)
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 10 instances of this issue:
File: src/NontransferableERC20Votes.sol 87: function decimals() public view virtual override returns (uint8) { 94: function transfer(address, uint256) public virtual override returns (bool) { 108: function transferFrom(address, address, uint256) public virtual override returns (bool) { 115: function approve(address, uint256) public virtual override returns (bool) {
File: src/VerbsToken.sol 177: function mint() public override onlyMinter nonReentrant returns (uint256) { 184: function burn(uint256 verbId) public override onlyMinter nonReentrant { 193: function tokenURI(uint256 tokenId) public view override returns (string memory) { 201: function dataURI(uint256 tokenId) public view override returns (string memory) {
File: src/libs/VRGDAC.sol 47: function xToY(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) { 54: function yToX(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) {
The functions below have the same implementation as is seen in other files. The functions should be refactored into functions of a common base contract
There is one instance of this issue:
File: src/MaxHeap.sol 181 function _authorizeUpgrade(address _newImpl) internal view override onlyOwner { 182 // Ensure the new implementation is a registered upgrade 183 if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl); 184: }
GitHub: 181
UpgradeableThis allows for bugs to be fixed in production, at the expense of significantly increasing centralization.
There is one instance of this issue:
File: src/libs/VRGDAC.sol 11 contract VRGDAC { 12 /*////////////////////////////////////////////////////////////// 13 VRGDA PARAMETERS 14 //////////////////////////////////////////////////////////////*/ 15:
GitHub: 11
struct rather than having many function input parametersOften times, a subset of the parameters would be more clear if they were passed as a struct
There are 2 instances of this issue:
File: src/AuctionHouse.sol 113 function initialize( 114 address _erc721Token, 115 address _erc20TokenEmitter, 116 address _initialOwner, 117 address _weth, 118 IRevolutionBuilder.AuctionParams calldata _auctionParams 119: ) external initializer {
GitHub: 113
File: src/VerbsToken.sol 130 function initialize( 131 address _minter, 132 address _initialOwner, 133 address _descriptor, 134 address _cultureIndex, 135 IRevolutionBuilder.ERC721TokenParams memory _erc721TokenParams 136: ) external initializer {
GitHub: 130
constants when passing zero as a function argumentPassing zero as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter). Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
There are 9 instances of this issue:
File: src/AuctionHouse.sol 319: amount: 0, 322: bidder: payable(0), 404: builder: address(0), 405: purchaseReferral: address(0),
File: src/ERC20TokenEmitter.sol 191: (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); 196: (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
File: src/MaxHeap.sol 161: maxHeapify(0);
GitHub: 161
File: src/NontransferableERC20Votes.sol 129: revert ERC20InvalidReceiver(address(0)); 131: _update(address(0), account, value);
When calling functions in external contracts with multiple arguments, consider using named function parameters, rather than positional ones.
There are 16 instances of this issue:
File: src/AuctionHouse.sol 400: creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }( 454: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
File: src/CultureIndex.sol 221: maxHeap.insert(pieceId, 0); 295: erc20VotingToken.getPastVotes(account, blockNumber), 296: erc721VotingToken.getPastVotes(account, blockNumber) 322: maxHeap.updateValue(pieceId, totalWeight); 545: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 221, 295, 296, 322, 545
File: src/ERC20TokenEmitter.sol 109: token.mint(_to, _amount); 242 vrgdac.xToY({ 243 timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime), 244 sold: emittedTokenWad, 245 amount: int(amount) 246: }); 259 vrgdac.yToX({ 260 timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime), 261 sold: emittedTokenWad, 262 amount: int(etherAmount) 263: }); 276 vrgdac.yToX({ 277 timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime), 278 sold: emittedTokenWad, 279 amount: int(((paymentAmount - computeTotalReward(paymentAmount)) * (10_000 - creatorRateBps)) / 10_000) 280: });
File: src/MaxHeap.sol 183: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 183
File: src/VerbsToken.sol 194: return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata); 202: return descriptor.dataURI(tokenId, artPieces[tokenId].metadata); 330: require(manager.isRegisteredUpgrade(_getImplementation(), _newImpl), "Invalid upgrade");
File: src/abstract/RewardSplits.sol 80: protocolRewards.depositRewards{ value: totalReward }(
GitHub: 80
Using named returns makes the code more self-documenting, makes it easier to fill out NatSpec, and in some cases can save gas. The cases below are where there currently is at most one return statement, which is ideal for named returns.
There are 38 instances of this issue:
<details> <summary>see instances</summary>File: src/CultureIndex.sol 179: function validateCreatorsArray(CreatorBps[] calldata creatorArray) internal pure returns (uint256) { 209 function createPiece( 210 ArtPieceMetadata calldata metadata, 211 CreatorBps[] calldata creatorArray 212: ) public returns (uint256) { 256: function hasVoted(uint256 pieceId, address voter) external view returns (bool) { 265: function getVotes(address account) external view override returns (uint256) { 274: function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) { 284: function _calculateVoteWeight(uint256 erc20Balance, uint256 erc721Balance) internal view returns (uint256) { 288: function _getVotes(address account) internal view returns (uint256) { 292: function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) { 451: function getPieceById(uint256 pieceId) public view returns (ArtPiece memory) { 461: function getVote(uint256 pieceId, address voter) public view returns (Vote memory) { 470: function getTopVotedPiece() public view returns (ArtPiece memory) { 478: function pieceCount() external view returns (uint256) { 486: function topVotedPieceId() public view returns (uint256) { 509: function quorumVotes() public view returns (uint256) { 519: function dropTopVotedPiece() public nonReentrant returns (ArtPiece memory) {
GitHub: 179, 209, 256, 265, 274, 284, 288, 292, 451, 461, 470, 478, 486, 509, 519
File: src/ERC20TokenEmitter.sol 112: function totalSupply() public view returns (uint) { 117: function decimals() public view returns (uint8) { 122: function balanceOf(address _owner) public view returns (uint) {
File: src/MaxHeap.sol 78: function parent(uint256 pos) private pure returns (uint256) { 156: function extractMax() external onlyAdmin returns (uint256, uint256) { 169: function getMax() public view returns (uint256, uint256) {
File: src/NontransferableERC20Votes.sol 87: function decimals() public view virtual override returns (uint8) { 94: function transfer(address, uint256) public virtual override returns (bool) { 108: function transferFrom(address, address, uint256) public virtual override returns (bool) { 115: function approve(address, uint256) public virtual override returns (bool) {
File: src/VerbsToken.sol 161: function contractURI() public view returns (string memory) { 177: function mint() public override onlyMinter nonReentrant returns (uint256) { 193: function tokenURI(uint256 tokenId) public view override returns (string memory) { 201: function dataURI(uint256 tokenId) public view override returns (string memory) { 273: function getArtPieceById(uint256 verbId) public view returns (ICultureIndex.ArtPiece memory) { 281: function _mintTo(address to) internal returns (uint256) {
GitHub: 161, 177, 193, 201, 273, 281
File: src/libs/VRGDAC.sol 47: function xToY(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) { 54: function yToX(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) { 86: function pIntegral(int256 timeSinceStart, int256 sold) internal view returns (int256) {
File: src/abstract/RewardSplits.sol 40: function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) { 54: function computePurchaseRewards(uint256 paymentAmountWei) public pure returns (RewardsSettings memory, uint256) { 66 function _depositPurchaseRewards( 67 uint256 paymentAmountWei, 68 address builderReferral, 69 address purchaseReferral, 70 address deployer 71: ) internal returns (uint256) {
File: src/abstract/TokenEmitter/TokenEmitterRewards.sol 12 function _handleRewardsAndGetValueToSend( 13 uint256 msgValue, 14 address builderReferral, 15 address purchaseReferral, 16 address deployer 17: ) internal returns (uint256) {
GitHub: 12
</details>revert()/require()Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.
There are 4 instances of this issue:
File: src/AuctionHouse.sol 421: if (address(this).balance < _amount) revert("Insufficient balance"); 441: if (!wethSuccess) revert("WETH transfer failed");
File: src/VerbsToken.sol 317: revert("dropTopVotedPiece failed");
GitHub: 317
File: src/abstract/RewardSplits.sol 30: if (_protocolRewards == address(0) || _revolutionRewardRecipient == address(0)) revert("Invalid Address Zero");
GitHub: 30
pureThere are 2 instances of this issue:
File: src/NontransferableERC20Votes.sol 101: function _transfer(address from, address to, uint256 value) internal override { 141: function _approve(address owner, address spender, uint256 value) internal override {
viewNote that when overriding, mutability is allowed to be changed to be more strict than the parent function's mutability
There are 2 instances of this issue:
File: src/NontransferableERC20Votes.sol 101: function _transfer(address from, address to, uint256 value) internal override { 141: function _approve(address owner, address spender, uint256 value) internal override {
Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability
There are 2 instances of this issue:
File: src/CultureIndex.sol /// @audit 6_000 48: uint256 public constant MAX_QUORUM_VOTES_BPS = 6_000; // 6,000 basis points or 60%
GitHub: 48
File: src/abstract/RewardSplits.sol /// @audit 50_000 24: uint256 public constant maxPurchaseAmount = 50_000 ether;
GitHub: 24
Events help non-contract tools to track changes
There are 3 instances of this issue:
File: src/MaxHeap.sol /// @audit valueMapping: insert() /// @audit heap: insert() 119: function insert(uint256 itemId, uint256 value) public onlyAdmin { /// @audit valueMapping: updateValue() 136: function updateValue(uint256 itemId, uint256 newValue) public onlyAdmin {
@dev tags@dev is used to explain extra details to developers
There are 2 instances of this issue:
File: src/NontransferableERC20Votes.sol 30: error TRANSFER_NOT_ALLOWED();
GitHub: 30
File: src/abstract/RewardSplits.sol 15: error INVALID_ETH_AMOUNT();
GitHub: 15
@notice tags@notice is used to explain to end users what the error does, and the compiler interprets /// or /** comments (but not // or /*) as this tag if one wasn't explicitly provided
There are 2 instances of this issue:
File: src/NontransferableERC20Votes.sol 30: error TRANSFER_NOT_ALLOWED();
GitHub: 30
File: src/abstract/RewardSplits.sol 15: error INVALID_ETH_AMOUNT();
GitHub: 15
@return tag is missingThere are 12 instances of this issue:
File: src/CultureIndex.sol /// @audit Missing '@return ' 288: function _getVotes(address account) internal view returns (uint256) { /// @audit Missing '@return ' 292: function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
File: src/ERC20TokenEmitter.sol /// @audit Missing '@return ' 112: function totalSupply() public view returns (uint) { /// @audit Missing '@return ' 117: function decimals() public view returns (uint8) { /// @audit Missing '@return ' 122: function balanceOf(address _owner) public view returns (uint) {
File: src/libs/VRGDAC.sol /// @audit Missing '@return ' 47: function xToY(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) { /// @audit Missing '@return ' 54: function yToX(int256 timeSinceStart, int256 sold, int256 amount) public view virtual returns (int256) { /// @audit Missing '@return ' 86: function pIntegral(int256 timeSinceStart, int256 sold) internal view returns (int256) {
File: src/abstract/RewardSplits.sol /// @audit Missing '@return ' 40: function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) { /// @audit Missing '@return ' 54: function computePurchaseRewards(uint256 paymentAmountWei) public pure returns (RewardsSettings memory, uint256) { /// @audit Missing '@return ' 66 function _depositPurchaseRewards( 67 uint256 paymentAmountWei, 68 address builderReferral, 69 address purchaseReferral, 70 address deployer 71: ) internal returns (uint256) {
File: src/abstract/TokenEmitter/TokenEmitterRewards.sol /// @audit Missing '@return ' 12 function _handleRewardsAndGetValueToSend( 13 uint256 msgValue, 14 address builderReferral, 15 address purchaseReferral, 16 address deployer 17: ) internal returns (uint256) {
GitHub: 12
@dev tagsi.e. @dev tags. Note that since they're non-public, @notice is not the right tag to use.
There are 5 instances of this issue:
File: src/CultureIndex.sol 85: IRevolutionBuilder private immutable manager;
GitHub: 85
File: src/ERC20TokenEmitter.sol 55: IRevolutionBuilder private immutable manager;
GitHub: 55
File: src/MaxHeap.sol 23: IRevolutionBuilder private immutable manager;
GitHub: 23
File: src/NontransferableERC20Votes.sol 37: IRevolutionBuilder private immutable manager;
GitHub: 37
File: src/VerbsToken.sol 109: IRevolutionBuilder private immutable manager;
GitHub: 109
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
There are 5 instances of this issue:
File: src/CultureIndex.sol /// @audit success 419 function _verifyVoteSignature( 420 address from, 421 uint256[] calldata pieceIds, 422 uint256 deadline, 423 uint8 v, 424 bytes32 r, 425 bytes32 s 426: ) internal returns (bool success) {
GitHub: 419
File: src/ERC20TokenEmitter.sol /// @audit tokensSoldWad 152 function buyToken( 153 address[] calldata addresses, 154 uint[] calldata basisPointSplits, 155 ProtocolRewardAddresses calldata protocolRewardsRecipients 156: ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) { /// @audit spentY 237: function buyTokenQuote(uint256 amount) public view returns (int spentY) { /// @audit gainedX 254: function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) { /// @audit gainedX 271: function getTokenQuoteForPayment(uint256 paymentAmount) external view returns (int gainedX) {
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
There are 5 instances of this issue:
File: src/CultureIndex.sol /// @audit success 419 function _verifyVoteSignature( 420 address from, 421 uint256[] calldata pieceIds, 422 uint256 deadline, 423 uint8 v, 424 bytes32 r, 425 bytes32 s 426: ) internal returns (bool success) {
GitHub: 419
File: src/ERC20TokenEmitter.sol /// @audit tokensSoldWad 152 function buyToken( 153 address[] calldata addresses, 154 uint[] calldata basisPointSplits, 155 ProtocolRewardAddresses calldata protocolRewardsRecipients 156: ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) { /// @audit spentY 237: function buyTokenQuote(uint256 amount) public view returns (int spentY) { /// @audit gainedX 254: function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) { /// @audit gainedX 271: function getTokenQuoteForPayment(uint256 paymentAmount) external view returns (int gainedX) {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There is one instance of this issue:
File: src/MaxHeap.sol /// @audit function constructor came earlier 41 modifier onlyAdmin() { 42 require(msg.sender == admin, "Sender is not the admin"); 43 _; 44: }
GitHub: 41
See the control structures section of the Solidity Style Guide
There are 27 instances of this issue:
File: src/AuctionHouse.sol 441: if (!wethSuccess) revert("WETH transfer failed"); 192: if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer; 195: if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount); 199: if (extended) emit AuctionExtended(_auction.verbId, _auction.endTime); 421: if (address(this).balance < _amount) revert("Insufficient balance"); 441: if (!wethSuccess) revert("WETH transfer failed"); 454: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 441, 192, 195, 199, 421, 441, 454
File: src/CultureIndex.sol 162 if (metadata.mediaType == MediaType.IMAGE) 163: require(bytes(metadata.image).length > 0, "Image URL must be provided"); 164 else if (metadata.mediaType == MediaType.ANIMATION) 165: require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided"); 166 else if (metadata.mediaType == MediaType.TEXT) 167: require(bytes(metadata.text).length > 0, "Text must be provided"); 377: if (!success) revert INVALID_SIGNATURE(); 377: if (!success) revert INVALID_SIGNATURE(); 404: if (!_verifyVoteSignature(from[i], pieceIds[i], deadline[i], v[i], r[i], s[i])) revert INVALID_SIGNATURE(); 438: if (from == address(0)) revert ADDRESS_ZERO(); 441: if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE(); 545: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 162, 164, 166, 377, 377, 404, 438, 441, 545
File: src/ERC20TokenEmitter.sol 188: if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;
GitHub: 188
File: src/MaxHeap.sol 102: if (pos >= (size / 2) && pos <= size) return; 150: } else if (newValue < oldValue) maxHeapify(position); // Downwards heapify 183: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
File: src/abstract/RewardSplits.sol 41: if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT(); 30: if (_protocolRewards == address(0) || _revolutionRewardRecipient == address(0)) revert("Invalid Address Zero"); 41: if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT(); 74: if (builderReferral == address(0)) builderReferral = revolutionRewardRecipient; 76: if (deployer == address(0)) deployer = revolutionRewardRecipient; 78: if (purchaseReferral == address(0)) purchaseReferral = revolutionRewardRecipient;
GitHub: 41, 30, 41, 74, 76, 78
File: src/abstract/TokenEmitter/TokenEmitterRewards.sol 18: if (msgValue < computeTotalReward(msgValue)) revert INVALID_ETH_AMOUNT();
GitHub: 18
See the whitespace section of the Solidity Style Guide
There are 59 instances of this issue:
<details> <summary>see instances</summary>File: src/AuctionHouse.sol 26: import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; 27: import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; 28: import { IAuctionHouse } from "./interfaces/IAuctionHouse.sol"; 29: import { IVerbsToken } from "./interfaces/IVerbsToken.sol"; 30: import { IWETH } from "./interfaces/IWETH.sol"; 31: import { IERC20TokenEmitter } from "./interfaces/IERC20TokenEmitter.sol"; 32: import { ICultureIndex } from "./interfaces/ICultureIndex.sol"; 33: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol"; 34: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 36: import { UUPS } from "./libs/proxy/UUPS.sol"; 37: import { VersionedContract } from "./version/VersionedContract.sol"; 400 creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }( 401: vrgdaReceivers, 435: IWETH(WETH).deposit{ value: _amount }();
GitHub: 26, 27, 28, 29, 30, 31, 32, 33, 34, 36, 37, 400, 435
File: src/CultureIndex.sol 4: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 5: import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; 7: import { UUPS } from "./libs/proxy/UUPS.sol"; 8: import { VersionedContract } from "./version/VersionedContract.sol"; 10: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol"; 12: import { ERC20VotesUpgradeable } from "./base/erc20/ERC20VotesUpgradeable.sol"; 13: import { MaxHeap } from "./MaxHeap.sol"; 14: import { ICultureIndex } from "./interfaces/ICultureIndex.sol"; 16: import { ERC721CheckpointableUpgradeable } from "./base/ERC721CheckpointableUpgradeable.sol"; 17: import { EIP712Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; 18: import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
GitHub: 4, 5, 7, 8, 10, 12, 13, 14, 16, 17, 18
File: src/ERC20TokenEmitter.sol 4: import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; 5: import { TokenEmitterRewards } from "@collectivexyz/protocol-rewards/src/abstract/TokenEmitter/TokenEmitterRewards.sol"; 6: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 7: import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; 9: import { VRGDAC } from "./libs/VRGDAC.sol"; 10: import { toDaysWadUnsafe } from "./libs/SignedWadMath.sol"; 11: import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; 12: import { NontransferableERC20Votes } from "./NontransferableERC20Votes.sol"; 13: import { IERC20TokenEmitter } from "./interfaces/IERC20TokenEmitter.sol"; 15: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol";
GitHub: 4, 5, 6, 7, 9, 10, 11, 12, 13, 15
File: src/MaxHeap.sol 4: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 5: import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; 6: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol"; 8: import { UUPS } from "./libs/proxy/UUPS.sol"; 9: import { VersionedContract } from "./version/VersionedContract.sol";
File: src/NontransferableERC20Votes.sol 20: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 21: import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; 22: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 24: import { ERC20VotesUpgradeable } from "./base/erc20/ERC20VotesUpgradeable.sol"; 25: import { EIP712Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; 27: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol";
GitHub: 20, 21, 22, 24, 25, 27
File: src/VerbsToken.sol 20: import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; 21: import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; 22: import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; 24: import { UUPS } from "./libs/proxy/UUPS.sol"; 25: import { VersionedContract } from "./version/VersionedContract.sol"; 27: import { ERC721CheckpointableUpgradeable } from "./base/ERC721CheckpointableUpgradeable.sol"; 28: import { IDescriptorMinimal } from "./interfaces/IDescriptorMinimal.sol"; 29: import { ICultureIndex } from "./interfaces/ICultureIndex.sol"; 30: import { IVerbsToken } from "./interfaces/IVerbsToken.sol"; 31: import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol";
GitHub: 20, 21, 22, 24, 25, 27, 28, 29, 30, 31
File: src/libs/VRGDAC.sol 4: import { wadExp, wadLn, wadMul, wadDiv, unsafeWadDiv, wadPow } from "./SignedWadMath.sol";
GitHub: 4
File: src/abstract/RewardSplits.sol 4: import { IRevolutionProtocolRewards } from "../interfaces/IRevolutionProtocolRewards.sol"; 80 protocolRewards.depositRewards{ value: totalReward }( 81: builderReferral,
File: src/abstract/TokenEmitter/TokenEmitterRewards.sol 4: import { RewardSplits } from "../RewardSplits.sol";
GitHub: 4
</details>external/public variable names should begin with an underscoreAccording to the Solidity Style Guide, non-external/public variable names should begin with an underscore
There are 11 instances of this issue:
File: src/CultureIndex.sol 85: IRevolutionBuilder private immutable manager;
GitHub: 85
File: src/ERC20TokenEmitter.sol 55: IRevolutionBuilder private immutable manager;
GitHub: 55
File: src/MaxHeap.sol 23: IRevolutionBuilder private immutable manager;
GitHub: 23
File: src/NontransferableERC20Votes.sol 37: IRevolutionBuilder private immutable manager;
GitHub: 37
File: src/VerbsToken.sol 109: IRevolutionBuilder private immutable manager;
GitHub: 109
File: src/abstract/RewardSplits.sol 18: uint256 internal constant DEPLOYER_REWARD_BPS = 25; 19: uint256 internal constant REVOLUTION_REWARD_BPS = 75; 20: uint256 internal constant BUILDER_REWARD_BPS = 100; 21: uint256 internal constant PURCHASE_REFERRAL_BPS = 50; 26: address internal immutable revolutionRewardRecipient; 27: IRevolutionProtocolRewards internal immutable protocolRewards;
GitHub: 18, 19, 20, 21, 26, 27
And functions within contracts should be separate by a single line
There is one instance of this issue:
File: src/NontransferableERC20Votes.sol 27 import { IRevolutionBuilder } from "./interfaces/IRevolutionBuilder.sol"; 28 29: contract NontransferableERC20Votes is Initializable, ERC20VotesUpgradeable, Ownable2StepUpgradeable {
GitHub: 27
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There is one instance of this issue:
File: src/VerbsToken.sol 232: ) external override onlyOwner nonReentrant whenDescriptorNotLocked {
GitHub: 232
_disableInitializers() in the constructor body, rather than using the initializer modifierThere are 6 instances of this issue:
File: src/AuctionHouse.sol 95: constructor(address _manager) payable initializer {
GitHub: 95
File: src/CultureIndex.sol 92: constructor(address _manager) payable initializer {
GitHub: 92
File: src/ERC20TokenEmitter.sol 64 constructor( 65 address _manager, 66 address _protocolRewards, 67 address _protocolFeeRecipient 68: ) payable TokenEmitterRewards(_protocolRewards, _protocolFeeRecipient) initializer {
GitHub: 64
File: src/MaxHeap.sol 30: constructor(address _manager) payable initializer {
GitHub: 30
File: src/NontransferableERC20Votes.sol 44: constructor(address _manager) payable initializer {
GitHub: 44
File: src/VerbsToken.sol 116: constructor(address _manager) payable initializer {
GitHub: 116
override is unnecessaryStarting with Solidity version 0.8.8, using the override keyword when the function solely overrides an interface function, and the function doesn't exist in multiple base contracts, is unnecessary.
There are 5 instances of this issue:
File: src/NontransferableERC20Votes.sol 87: function decimals() public view virtual override returns (uint8) { 94: function transfer(address, uint256) public virtual override returns (bool) { 108: function transferFrom(address, address, uint256) public virtual override returns (bool) { 115: function approve(address, uint256) public virtual override returns (bool) {
File: src/VerbsToken.sol 193: function tokenURI(uint256 tokenId) public view override returns (string memory) {
GitHub: 193
The default value for variables is zero, so initializing them to zero is superfluous.
There are 7 instances of this issue:
File: src/AuctionHouse.sol 346: uint256 creatorTokensEmitted = 0; 380: uint256 ethPaidToCreators = 0; 384: for (uint256 i = 0; i < numCreators; i++) {
File: src/ERC20TokenEmitter.sol 205: uint256 bpsSum = 0; 209: for (uint256 i = 0; i < addresses.length; i++) {
File: src/MaxHeap.sol 67: uint256 public size = 0;
GitHub: 67
File: src/VerbsToken.sol 306: for (uint i = 0; i < artPiece.creators.length; i++) {
GitHub: 306
#0 - thebrittfactor
2023-12-21T19:25:32Z
Added submission at warden request prior to audit close.
#1 - c4-pre-sort
2023-12-24T18:01:27Z
raymondfam marked the issue as sufficient quality report
#2 - c4-judge
2024-01-07T19:55:29Z
MarioPoneder marked the issue as grade-b
🌟 Selected for report: MrPotatoMagic
Also found by: 0x11singh99, 0xAnah, IllIllI, JCK, Raihan, SAQ, Sathish9098, SovaSlava, c3phas, donkicha, fnanni, hunter_w3b, lsaudit, naman1778, pavankv, sivanesh_808
20.8446 USDC - $20.84
Note: I've removed the specific instances from the bot race and 4naly3er
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | "" has the same value as new bytes(0) but costs less gas | 2 | 518 |
| [G‑02] | Assembly: Use scratch space for building calldata | 14 | 3080 |
| [G‑03] | Avoid transferring amounts of zero in order to save gas | 1 | 100 |
| [G‑04] | Combine mappings referenced in the same function by the same key | 4 | 168 |
| [G‑05] | Assembly: Use scratch space when building emitted events with two data arguments | 4 | 60 |
| [G‑06] | >= costs less gas than > | 9 | 27 |
| [G‑07] | Stack variable is only used once | 5 | 15 |
| [G‑08] | require()/revert() strings longer than 32 bytes cost extra gas | 1 | 3 |
| [G‑09] | Consider using solady's token contracts to save gas | 1 | - |
| [G‑10] | Consider using solady's FixedPointMathLib | 18 | - |
| [G‑11] | Duplicated require()/revert() checks should be refactored to a modifier or function to save deployment gas | 20 | - |
| [G‑12] | Events should be emitted outside of loops | 1 | 375 |
| [G‑13] | Multiple accesses of a memory/calldata array should use a local variable cache | 1 | - |
| [G‑14] | Reduce deployment costs by tweaking contracts' metadata | 7 | - |
| [G‑15] | Split revert checks to save gas | 2 | 4 |
| [G‑16] | Use internal functions inside modifiers to save gas | 5 | - |
| [G‑17] | Using private rather than public, saves gas | 44 | - |
Total: 139 instances over 17 issues with 4350 gas saved
Gas totals are estimates based on data from the Ethereum Yellowpaper. The estimates use the lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
"" has the same value as new bytes(0) but costs less gasThere are 2 instances of this issue:
File: src/ERC20TokenEmitter.sol 191: (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); 196: (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
If an external call's calldata can fit into two or fewer words, use the scratch space to build the calldata, rather than allowing Solidity to do a memory expansion.
There are 14 instances of this issue:
File: src/AuctionHouse.sol 355: verbs.burn(_auction.verbId); 359: verbs.burn(_auction.verbId); 370: uint256 numCreators = verbs.getArtPieceById(_auction.verbId).creators.length; 371: address deployer = verbs.getArtPieceById(_auction.verbId).sponsor; 385: ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i]; 438: bool wethSuccess = IWETH(WETH).transfer(_to, _amount); 454: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 355, 359, 370, 371, 385, 438, 454
File: src/CultureIndex.sol 289: return _calculateVoteWeight(erc20VotingToken.getVotes(account), erc721VotingToken.getVotes(account)); 294 _calculateVoteWeight( 295 erc20VotingToken.getPastVotes(account, blockNumber), 296 erc721VotingToken.getPastVotes(account, blockNumber) 297: ); 545: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
File: src/ERC20TokenEmitter.sol 109: token.mint(_to, _amount); 124: return token.balanceOf(_owner);
File: src/MaxHeap.sol 183: if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
GitHub: 183
File: src/VerbsToken.sol 330: require(manager.isRegisteredUpgrade(_getImplementation(), _newImpl), "Invalid upgrade");
GitHub: 330
Skipping the external call when nothing will be transferred, will save at least 100 gas
There is one instance of this issue:
File: src/AuctionHouse.sol 438: bool wethSuccess = IWETH(WETH).transfer(_to, _amount);
GitHub: 438
mappings referenced in the same function by the same keyReads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot (i.e. runtime gas savings). Even if the values can't be packed, if both fields are accessed in the same function (as is the case for these instances), combining them can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There are 4 instances of this issue:
File: src/CultureIndex.sol /// @audit combine into a `struct`: pieces,totalVoteWeights,votes 307 function _vote(uint256 pieceId, address voter) internal { 308 require(pieceId < _currentPieceId, "Invalid piece ID"); 309 require(voter != address(0), "Invalid voter address"); 310 require(!pieces[pieceId].isDropped, "Piece has already been dropped"); 311 require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted"); 312 313 uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock); 314 require(weight > minVoteWeight, "Weight must be greater than minVoteWeight"); 315 316 votes[pieceId][voter] = Vote(voter, weight); 317 totalVoteWeights[pieceId] += weight; 318 319 uint256 totalWeight = totalVoteWeights[pieceId]; 320 321 // TODO add security consideration here based on block created to prevent flash attacks on drops? 322 maxHeap.updateValue(pieceId, totalWeight); 323 emit VoteCast(pieceId, voter, weight, totalWeight); 324: } /// @audit combine into a `struct`: pieces,totalVoteWeights 519 function dropTopVotedPiece() public nonReentrant returns (ArtPiece memory) { 520 require(msg.sender == dropperAdmin, "Only dropper can drop pieces"); 521 522 ICultureIndex.ArtPiece memory piece = getTopVotedPiece(); 523 require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped."); 524 525 //set the piece as dropped 526 pieces[piece.pieceId].isDropped = true; 527 528 //slither-disable-next-line unused-return 529 maxHeap.extractMax(); 530 531 emit PieceDropped(piece.pieceId, msg.sender); 532 533 return pieces[piece.pieceId]; 534: }
File: src/MaxHeap.sol /// @audit combine into a `struct`: positionMapping,valueMapping 119 function insert(uint256 itemId, uint256 value) public onlyAdmin { 120 heap[size] = itemId; 121 valueMapping[itemId] = value; // Update the value mapping 122 positionMapping[itemId] = size; // Update the position mapping 123 124 uint256 current = size; 125 while (current != 0 && valueMapping[heap[current]] > valueMapping[heap[parent(current)]]) { 126 swap(current, parent(current)); 127 current = parent(current); 128 } 129 size++; 130: } /// @audit combine into a `struct`: positionMapping,valueMapping 136 function updateValue(uint256 itemId, uint256 newValue) public onlyAdmin { 137 uint256 position = positionMapping[itemId]; 138 uint256 oldValue = valueMapping[itemId]; 139 140 // Update the value in the valueMapping 141 valueMapping[itemId] = newValue; 142 143 // Decide whether to perform upwards or downwards heapify 144 if (newValue > oldValue) { 145 // Upwards heapify 146 while (position != 0 && valueMapping[heap[position]] > valueMapping[heap[parent(position)]]) { 147 swap(position, parent(position)); 148 position = parent(position); 149 } 150 } else if (newValue < oldValue) maxHeapify(position); // Downwards heapify 151: }
Using the scratch space for more than one, but at most two words worth of data (non-indexed arguments) will save gas over needing Solidity's abi memory expansion used for emitting normally.
There are 4 instances of this issue:
File: src/AuctionHouse.sol 326: emit AuctionCreated(verbId, startTime, endTime);
GitHub: 326
File: src/CultureIndex.sol 141: emit QuorumVotesBPSSet(quorumVotesBPS, _cultureIndexParams.quorumVotesBPS); 323: emit VoteCast(pieceId, voter, weight, totalWeight); 500: emit QuorumVotesBPSSet(quorumVotesBPS, newQuorumVotesBPS);
>= costs less gas than >The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas. If < is being used, the condition can be inverted. In cases where a for-loop is being used, one can count down rather than up, in order to use the optimal operator
There are 9 instances of this issue:
File: src/AuctionHouse.sol 384: for (uint256 i = 0; i < numCreators; i++) {
GitHub: 384
File: src/CultureIndex.sol 185: for (uint i; i < creatorArrayLength; i++) { 236: for (uint i; i < creatorArrayLength; i++) { 243: for (uint i; i < creatorArrayLength; i++) { 355: for (uint256 i; i < len; i++) { 403: for (uint256 i; i < len; i++) { 407: for (uint256 i; i < len; i++) {
GitHub: 185, 236, 243, 355, 403, 407
File: src/ERC20TokenEmitter.sol 209: for (uint256 i = 0; i < addresses.length; i++) {
GitHub: 209
File: src/VerbsToken.sol 306: for (uint i = 0; i < artPiece.creators.length; i++) {
GitHub: 306
If the variable is only accessed once, it's cheaper to use the assigned value directly that one time, and save the 3 gas the extra stack assignment would spend
There are 5 instances of this issue:
File: src/AuctionHouse.sol 371: address deployer = verbs.getArtPieceById(_auction.verbId).sponsor; 438: bool wethSuccess = IWETH(WETH).transfer(_to, _amount);
File: src/CultureIndex.sol 375: bool success = _verifyVoteSignature(from, pieceIds, deadline, v, r, s); 433: bytes32 digest = _hashTypedDataV4(voteHash); 489: (uint256 pieceId, ) = maxHeap.getMax();
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There is one instance of this issue:
File: src/AuctionHouse.sol 340: require(!_auction.settled, "Auction has already been settled");
GitHub: 340
solady's token contracts to save gasThey're written using heavily-optimized assembly, and will your users a lot of gas
There is one instance of this issue:
File: src/VerbsToken.sol /// @audit ERC721Upgradeable 33 contract VerbsToken is 34 IVerbsToken, 35 VersionedContract, 36 UUPS, 37 Ownable2StepUpgradeable, 38 ReentrancyGuardUpgradeable, 39 ERC721CheckpointableUpgradeable 40 { 41: // An address who has permissions to mint Verbs
GitHub: 33
FixedPointMathLibSaves gas, and works to avoid unnecessary overflows.
There are 18 instances of this issue:
File: src/AuctionHouse.sol 181: msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), 365: uint256 auctioneerPayment = (_auction.amount * (10_000 - creatorRateBps)) / 10_000; 390: uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
File: src/CultureIndex.sol 234: newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; 511 (quorumVotesBPS * _calculateVoteWeight(erc20VotingToken.totalSupply(), erc721VotingToken.totalSupply())) / 512: 10_000;
File: src/ERC20TokenEmitter.sol 173: uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000; 177: uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000; 212: _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000)); 279: amount: int(((paymentAmount - computeTotalReward(paymentAmount)) * (10_000 - creatorRateBps)) / 10_000)
File: src/MaxHeap.sol 80: return (pos - 1) / 2;
GitHub: 80
File: src/abstract/RewardSplits.sol 44 (paymentAmountWei * BUILDER_REWARD_BPS) / 45: 10_000 + 46 (paymentAmountWei * PURCHASE_REFERRAL_BPS) / 47: 10_000 + 48 (paymentAmountWei * DEPLOYER_REWARD_BPS) / 49: 10_000 + 50 (paymentAmountWei * REVOLUTION_REWARD_BPS) / 51: 10_000; 57: builderReferralReward: (paymentAmountWei * BUILDER_REWARD_BPS) / 10_000, 58: purchaseReferralReward: (paymentAmountWei * PURCHASE_REFERRAL_BPS) / 10_000, 59: deployerReward: (paymentAmountWei * DEPLOYER_REWARD_BPS) / 10_000, 60: revolutionReward: (paymentAmountWei * REVOLUTION_REWARD_BPS) / 10_000
GitHub: 44, 46, 48, 50, 57, 58, 59, 60
require()/revert() checks should be refactored to a modifier or function to save deployment gasThis will cost more runtime gas, but will reduce deployment gas when the function is (optionally called via a modifier) called more than once as is the case for the examples below. Most projects do not make this trade-off, but it's available nonetheless.
There are 20 instances of this issue:
File: src/AuctionHouse.sol 120: require(msg.sender == address(manager), "Only manager can initialize"); 222: require(_creatorRateBps <= 10_000, "Creator rate must be less than or equal to 10_000"); 254: require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");
File: src/CultureIndex.sol 117: require(msg.sender == address(manager), "Only manager can initialize"); 308: require(pieceId < _currentPieceId, "Invalid piece ID"); 452: require(pieceId < _currentPieceId, "Invalid piece ID"); 462: require(pieceId < _currentPieceId, "Invalid piece ID");
File: src/ERC20TokenEmitter.sol 91: require(msg.sender == address(manager), "Only manager can initialize"); 192: require(success, "Transfer failed."); 197: require(success, "Transfer failed."); 289: require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000"); 300: require(_creatorRateBps <= 10_000, "Creator rate must be less than or equal to 10_000");
GitHub: 91, 192, 197, 289, 300
File: src/MaxHeap.sol 56: require(msg.sender == address(manager), "Only manager can initialize"); 157: require(size > 0, "Heap is empty"); 170: require(size > 0, "Heap is empty");
File: src/NontransferableERC20Votes.sol 69: require(msg.sender == address(manager), "Only manager can initialize");
GitHub: 69
File: src/VerbsToken.sol 137: require(msg.sender == address(manager), "Only manager can initialize"); 139: require(_minter != address(0), "Minter cannot be zero address"); 140: require(_initialOwner != address(0), "Initial owner cannot be zero address"); 210: require(_minter != address(0), "Minter cannot be zero address");
Emitting an event has an overhead of 375 gas, which will be incurred on every iteration of the loop. It is cheaper to emit only once after the loop has finished.
There is one instance of this issue:
File: src/CultureIndex.sol /// @audit _vote(), _voteForMany() 323: emit VoteCast(pieceId, voter, weight, totalWeight);
GitHub: 323
memory/calldata array should use a local variable cacheThe instances below point to the second+ access of a value inside a memory/calldata array, within a function. Caching avoids recalculating the array offsets into memory/calldata
There is one instance of this issue:
File: src/CultureIndex.sol /// @audit creatorArray...[] 187: totalBps += creatorArray[i].bps;
GitHub: 187
See this link, at its bottom, for full details
There are 7 instances of this issue:
<details> <summary>see instances</summary>File: src/AuctionHouse.sol 39 contract AuctionHouse is 40 IAuctionHouse, 41 VersionedContract, 42 UUPS, 43 PausableUpgradeable, 44 ReentrancyGuardUpgradeable, 45 Ownable2StepUpgradeable 46 { 47: // The Verbs ERC721 token contract
GitHub: 39
File: src/CultureIndex.sol 20 contract CultureIndex is 21 ICultureIndex, 22 VersionedContract, 23 UUPS, 24 Ownable2StepUpgradeable, 25 ReentrancyGuardUpgradeable, 26 EIP712Upgradeable 27 { 28: /// @notice The EIP-712 typehash for gasless votes
GitHub: 20
File: src/ERC20TokenEmitter.sol 17 contract ERC20TokenEmitter is 18 IERC20TokenEmitter, 19 ReentrancyGuardUpgradeable, 20 TokenEmitterRewards, 21 Ownable2StepUpgradeable, 22 PausableUpgradeable 23 { 24: // treasury address to pay funds to
GitHub: 17
File: src/MaxHeap.sol 14 contract MaxHeap is VersionedContract, UUPS, Ownable2StepUpgradeable, ReentrancyGuardUpgradeable { 15: /// @notice The parent contract that is allowed to update the data store
GitHub: 14
File: src/NontransferableERC20Votes.sol 29: contract NontransferableERC20Votes is Initializable, ERC20VotesUpgradeable, Ownable2StepUpgradeable {
GitHub: 29
File: src/VerbsToken.sol 33 contract VerbsToken is 34 IVerbsToken, 35 VersionedContract, 36 UUPS, 37 Ownable2StepUpgradeable, 38 ReentrancyGuardUpgradeable, 39 ERC721CheckpointableUpgradeable 40 { 41: // An address who has permissions to mint Verbs
GitHub: 33
File: src/libs/VRGDAC.sol 11 contract VRGDAC { 12 /*////////////////////////////////////////////////////////////// 13 VRGDA PARAMETERS 14 //////////////////////////////////////////////////////////////*/ 15:
GitHub: 11
</details>revert checks to save gasSplitting the conditions into two separate checks saves 2 gas
There are 2 instances of this issue:
File: src/CultureIndex.sol 441: if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE();
GitHub: 441
File: src/abstract/RewardSplits.sol 41: if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT();
GitHub: 41
This will cost more runtime gas, but will reduce deployment gas when the modifier is called more than once as is the case for the examples below. Most projects do not make this trade-off, but it's available nonetheless.
There are 5 instances of this issue:
File: src/MaxHeap.sol 41 modifier onlyAdmin() { 42 require(msg.sender == admin, "Sender is not the admin"); 43 _; 44: }
GitHub: 41
File: src/VerbsToken.sol 75 modifier whenMinterNotLocked() { 76 require(!isMinterLocked, "Minter is locked"); 77 _; 78: } 83 modifier whenCultureIndexNotLocked() { 84 require(!isCultureIndexLocked, "CultureIndex is locked"); 85 _; 86: } 91 modifier whenDescriptorNotLocked() { 92 require(!isDescriptorLocked, "Descriptor is locked"); 93 _; 94: } 99 modifier onlyMinter() { 100 require(msg.sender == minter, "Sender is not the minter"); 101 _; 102: }
private rather than public, saves gasFor constants, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 44 instances of this issue:
File: src/AuctionHouse.sol 48: IVerbsToken public verbs; 51: IERC20TokenEmitter public erc20TokenEmitter; 54: address public WETH; 57: uint256 public timeBuffer; 60: uint256 public reservePrice; 63: uint8 public minBidIncrementPercentage; 66: uint256 public creatorRateBps; 69: uint256 public minCreatorRateBps; 72: uint256 public entropyRateBps; 75: uint256 public duration; 78: IAuctionHouse.Auction public auction;
GitHub: 48, 51, 54, 57, 60, 63, 66, 69, 72, 75, 78
File: src/CultureIndex.sol 33: mapping(address => uint256) public nonces; 36: MaxHeap public maxHeap; 39: ERC20VotesUpgradeable public erc20VotingToken; 42: ERC721CheckpointableUpgradeable public erc721VotingToken; 45: uint256 public erc721VotingTokenWeight; 51: uint256 public minVoteWeight; 54: uint256 public quorumVotesBPS; 60: string public description; 63: mapping(uint256 => ArtPiece) public pieces; 66: uint256 public _currentPieceId; 69: mapping(uint256 => mapping(address => Vote)) public votes; 72: mapping(uint256 => uint256) public totalVoteWeights; 78: address public dropperAdmin;
GitHub: 33, 36, 39, 42, 45, 51, 54, 60, 63, 66, 69, 72, 78
File: src/ERC20TokenEmitter.sol 25: address public treasury; 28: NontransferableERC20Votes public token; 31: VRGDAC public vrgdac; 34: uint256 public startTime; 39: int256 public emittedTokenWad; 42: uint256 public creatorRateBps; 45: uint256 public entropyRateBps; 48: address public creatorsAddress;
GitHub: 25, 28, 31, 34, 39, 42, 45, 48
File: src/MaxHeap.sol 16: address public admin; 65: mapping(uint256 => uint256) public heap; 67: uint256 public size = 0; 70: mapping(uint256 => uint256) public valueMapping; 73: mapping(uint256 => uint256) public positionMapping;
File: src/VerbsToken.sol 42: address public minter; 45: IDescriptorMinimal public descriptor; 48: ICultureIndex public cultureIndex; 51: bool public isMinterLocked; 54: bool public isCultureIndexLocked; 57: bool public isDescriptorLocked; 66: mapping(uint256 => ICultureIndex.ArtPiece) public artPieces;
#0 - thebrittfactor
2023-12-21T19:27:01Z
Added submission at warden request prior to audit close.
#1 - c4-pre-sort
2023-12-24T01:53:57Z
raymondfam marked the issue as high quality report
#2 - c4-sponsor
2024-01-05T00:02:03Z
rocketman-21 (sponsor) acknowledged
#3 - c4-judge
2024-01-07T13:41:07Z
MarioPoneder marked the issue as grade-a
#4 - c4-judge
2024-01-07T13:44:41Z
MarioPoneder marked the issue as grade-b