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: 61/110
Findings: 2
Award: $46.47
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
| issue | instance | |
|---|---|---|
| [G‑01] | State variables should be cached in stack variables rather than re-reading them from storage | 5 |
| [G‑02] | Multiple accesses of a mapping/array should use a local variable cache | 6 |
| [G‑03] | Multiple Address/id Mappings Can Be Combined Into A Single Mapping Of An Address/id To A Struct, Where Appropriate | 1 |
| [G‑04] | Avoid transferring amounts of zero in order to save gas | 1 |
| [G‑05] | Optimize External Calls with Assembly for Memory Efficiency | 2 |
| [G‑06] | Sort Solidity operations using short-circuit mode | 1 |
| [G‑07] | Do not shrink Variables | 1 |
| [G‑08] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 1 |
| [G‑09] | Most important: avoid zero to one storage writes where possible | 1 |
| [G‑10] | Use assembly to perform efficient back-to-back calls | 2 |
| [G‑11] | keccak256() should only need to be called on a specific string literal once | 1 |
| [G‑12] | Use selfbalance() instead of address(this).balance | 2 |
| [G‑13] | Cache external calls outside of loop to avoid re-calling function on each iteration | 1 |
| [G‑14] | "" has the same value as new bytes(0) but costs less gas | 2 |
| [G‑15] | Unlimited gas consumption risk due to external call recipients | 2 |
| [G‑16] | When variables are declared with the storage keyword ,caching any fields that need to be re-read in stack variables Saves gas | 2 |
| [G‑17] | Keep strings smaller than 32 bytes | 1 |
| [G‑18] | Use fallback or receive instead of deposit() when transferring Ether | 1 |
| [G‑19] | Using assembly to revert with an error message | 78 |
| [G‑20] | Split revert statements | 2 |
| [G‑21] | It is sometimes cheaper to cache calldata | 1 |
| [G‑22] | Don’t cache calls that are only used once to save gas | 1 |
| [G‑23] | Change public state variable visibility to private | 3 |
| [G‑24] | Use uint256(1)/uint256(2) instead for true and false boolean states | 3 |
| [G‑25] | Understand the trade-offs when choosing between internal functions and modifiers | 4 |
| [G‑26] | Using private for immutable to saves gas | 6 |
| [G‑27] | Move storage pointer to top of function to avoid offset calculation | 1 |
| [G‑28] | Use assembly for math (add, sub, mul, div) | 1 |
| [G‑29] | Save gas by preventing zero amount in mint() and burn() | 2 |
| [G‑30] | Do not perform depositRewards when totalReward is 0 to save gas | 1 |
| [G‑31] | Do not perform deposit when _amount is 0 to save gas | 1 |
| [G‑32] | Refactor a modifier to call a local function instead of directly having the code in the modifier, saving bytecode size and thereby deployment cost | 5 |
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
File: packages/revolution/src/AuctionHouse.sol 191 bool extended = _auction.endTime - block.timestamp < timeBuffer; 192 if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;
function createBid(uint256 verbId, address bidder) external payable override nonReentrant { IAuctionHouse.Auction memory _auction = auction; //require bidder is valid address require(bidder != address(0), "Bidder cannot be zero address"); require(_auction.verbId == verbId, "Verb not up for auction"); //slither-disable-next-line timestamp require(block.timestamp < _auction.endTime, "Auction expired"); require(msg.value >= reservePrice, "Must send at least reservePrice"); require( msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), "Must send more than last bid by minBidIncrementPercentage amount" ); address payable lastBidder = _auction.bidder; auction.amount = msg.value; auction.bidder = payable(bidder); + uint256 _timeBuffer = timeBuffer; // Extend the auction if the bid was received within `timeBuffer` of the auction end time - bool extended = _auction.endTime - block.timestamp < timeBuffer; + bool extended = _auction.endTime - block.timestamp < _timeBuffer; - if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer; + if (extended) auction.endTime = _auction.endTime = block.timestamp + _timeBuffer; // Refund the last bidder, if applicable if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount); emit AuctionBid(_auction.verbId, bidder, msg.sender, msg.value, extended); if (extended) emit AuctionExtended(_auction.verbId, _auction.endTime); }
File: packages/revolution/src/AuctionHouse.sol 383 if (creatorsShare > 0 && entropyRateBps > 0) { 390 uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
function _settleAuction() internal { IAuctionHouse.Auction memory _auction = auction; require(_auction.startTime != 0, "Auction hasn't begun"); require(!_auction.settled, "Auction has already been settled"); //slither-disable-next-line timestamp require(block.timestamp >= _auction.endTime, "Auction hasn't completed"); auction.settled = true; uint256 creatorTokensEmitted = 0; // Check if contract balance is greater than reserve price if (address(this).balance < reservePrice) { // If contract balance is less than reserve price, refund to the last bidder if (_auction.bidder != address(0)) { _safeTransferETHWithFallback(_auction.bidder, _auction.amount); } // And then burn the Noun verbs.burn(_auction.verbId); } else { //If no one has bid, burn the Verb if (_auction.bidder == address(0)) verbs.burn(_auction.verbId); //If someone has bid, transfer the Verb to the winning bidder else verbs.transferFrom(address(this), _auction.bidder, _auction.verbId); if (_auction.amount > 0) { // Ether going to owner of the auction uint256 auctioneerPayment = (_auction.amount * (10_000 - creatorRateBps)) / 10_000; //Total amount of ether going to creator uint256 creatorsShare = _auction.amount - auctioneerPayment; uint256 numCreators = verbs.getArtPieceById(_auction.verbId).creators.length; address deployer = verbs.getArtPieceById(_auction.verbId).sponsor; //Build arrays for erc20TokenEmitter.buyToken uint256[] memory vrgdaSplits = new uint256[](numCreators); address[] memory vrgdaReceivers = new address[](numCreators); //Transfer auction amount to the DAO treasury _safeTransferETHWithFallback(owner(), auctioneerPayment); uint256 ethPaidToCreators = 0; + // Cache the entropyRateBps state variable + uint256 entropyRate = entropyRateBps; //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken - if (creatorsShare > 0 && entropyRateBps > 0) { + if (creatorsShare > 0 && entropyRate > 0) { for (uint256 i = 0; i < numCreators; i++) { ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i]; vrgdaReceivers[i] = creator.creator; vrgdaSplits[i] = creator.bps; //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment - uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000); + uint256 paymentAmount = (creatorsShare * entropyRate * creator.bps) / (10_000 * 10_000); ethPaidToCreators += paymentAmount; //Transfer creator's share to the creator _safeTransferETHWithFallback(creator.creator, paymentAmount); } } //Buy token from ERC20TokenEmitter for all the creators if (creatorsShare > ethPaidToCreators) { creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }( vrgdaReceivers, vrgdaSplits, IERC20TokenEmitter.ProtocolRewardAddresses({ builder: address(0), purchaseReferral: address(0), deployer: deployer }) ); } } } emit AuctionSettled(_auction.verbId, _auction.bidder, _auction.amount, creatorTokensEmitted); }
File: packages/revolution/src/ERC20TokenEmitter.sol 158 require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens"); 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0)); 201 if (totalTokensForCreators > 0 && creatorsAddress != address(0)) { 202 _mint(creatorsAddress, uint256(totalTokensForCreators));
function buyToken( address[] calldata addresses, uint[] calldata basisPointSplits, ProtocolRewardAddresses calldata protocolRewardsRecipients ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) { + // Cache the creatorsAddress state variable + address creatorsAddr = creatorsAddress; //prevent treasury from paying itself - require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens"); + require(msg.sender != treasury && msg.sender != creatorsAddr, "Funds recipient cannot buy tokens"); require(msg.value > 0, "Must send ether"); // ensure the same number of addresses and bps require(addresses.length == basisPointSplits.length, "Parallel arrays required"); // Get value left after protocol rewards uint256 msgValueRemaining = _handleRewardsAndGetValueToSend( msg.value, protocolRewardsRecipients.builder, protocolRewardsRecipients.purchaseReferral, protocolRewardsRecipients.deployer ); //Share of purchase amount to send to treasury uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000; //Share of purchase amount to reserve for creators //Ether directly sent to creators uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000; //Tokens to emit to creators int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0 ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment) : int(0); // Tokens to emit to buyers int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0); //Transfer ETH to treasury and update emitted emittedTokenWad += totalTokensForBuyers; if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators; //Deposit funds to treasury (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); require(success, "Transfer failed."); //Transfer ETH to creators if (creatorDirectPayment > 0) { - (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0)); + (success, ) = creatorsAddr.call{ value: creatorDirectPayment }(new bytes(0)); require(success, "Transfer failed."); } //Mint tokens for creators - if (totalTokensForCreators > 0 && creatorsAddress != address(0)) { + if (totalTokensForCreators > 0 && creatorsAddr != address(0)) { - _mint(creatorsAddress, uint256(totalTokensForCreators)); + _mint(creatorsAddr, uint256(totalTokensForCreators)); } uint256 bpsSum = 0; //Mint tokens to buyers for (uint256 i = 0; i < addresses.length; i++) { if (totalTokensForBuyers > 0) { // transfer tokens to address _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000)); } bpsSum += basisPointSplits[i]; } require(bpsSum == 10_000, "bps must add up to 10_000"); emit PurchaseFinalized( msg.sender, msg.value, toPayTreasury, msg.value - msgValueRemaining, uint256(totalTokensForBuyers), uint256(totalTokensForCreators), creatorDirectPayment ); return uint256(totalTokensForBuyers); }
File: packages/revolution/src/MaxHeap.sol 102 if (pos >= (size / 2) && pos <= size) return;
function maxHeapify(uint256 pos) internal { + uint256 heapSize = size; // Cache the size state variable uint256 left = 2 * pos + 1; uint256 right = 2 * pos + 2; uint256 posValue = valueMapping[heap[pos]]; uint256 leftValue = valueMapping[heap[left]]; uint256 rightValue = valueMapping[heap[right]]; - if (pos >= (size / 2) && pos <= size) return; + if (pos >= (heapSize / 2) && pos <= heapSize) return; if (posValue < leftValue || posValue < rightValue) { if (leftValue > rightValue) { swap(pos, left); maxHeapify(left); } else { swap(pos, right); maxHeapify(right); } } }
File: packages/revolution/src/MaxHeap.sol heap[size] = itemId; valueMapping[itemId] = value; // Update the value mapping positionMapping[itemId] = size; // Update the position mapping uint256 current = size;
function insert(uint256 itemId, uint256 value) public onlyAdmin { + uint256 heapSize = size; // Cache the size state variable - heap[size] = itemId; + heap[heapSize] = itemId; valueMapping[itemId] = value; // Update the value mapping - positionMapping[itemId] = size; // Update the position mapping + positionMapping[itemId] = heapSize; // Update the position mapping - uint256 current = size; + uint256 current = heapSize; while (current != 0 && valueMapping[heap[current]] > valueMapping[heap[parent(current)]]) { swap(current, parent(current)); current = parent(current); } - size++; + size = heapSize + 1; // Update the size state variable }
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~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. Caching an array's struct avoids recalculating the array offsets into memory/calldata
File: packages/revolution/src/MaxHeap.sol 98 uint256 posValue = valueMapping[heap[pos]]; 99 uint256 leftValue = valueMapping[heap[left]]; 100 uint256 rightValue = valueMapping[heap[right]];
File: packages/revolution/src/MaxHeap.sol uint256 oldValue = valueMapping[itemId]; // Update the value in the valueMapping valueMapping[itemId] = newValue; // Decide whether to perform upwards or downwards heapify if (newValue > oldValue) { // Upwards heapify while (position != 0 && valueMapping[heap[position]] > valueMapping[heap[parent(position)]]) {
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, 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.
File: packages/revolution/src/MaxHeap.sol 65 mapping(uint256 => uint256) public heap; 67 uint256 public size = 0; /// @notice Mapping to keep track of the value of an item in the heap 70 mapping(uint256 => uint256) public valueMapping; /// @notice Mapping to keep track of the position of an item in the heap 73 mapping(uint256 => uint256) public positionMapping;
struct MappingData { uint256 heapValue; uint256 valueMappingValue; uint256 positionMappingValue; } mapping(uint256 => MappingData) public combinedMapping;
Skipping the external call when nothing will be transferred, will save at least 100 gas
File: packages/revolution/src/AuctionHouse.sol 438 bool wethSuccess = IWETH(WETH).transfer(_to, _amount);
Using interfaces to make external contract calls in Solidity is convenient but can be inefficient in terms of memory utilization. Each such call involves creating a new memory location to store the data being passed, thus incurring memory expansion costs. Inline assembly allows for optimized memory usage by re-using already allocated memory spaces or using the scratch space for smaller datasets. This can result in notable gas savings, especially for contracts that make frequent external calls. Additionally, using inline assembly enables important safety checks like verifying if the target address has code deployed to it using extcodesize(addr) before making the call, mitigating risks associated with contract interactions.Reffrence
AuctionHouse._settleAuction functionFile: packages/revolution/src/AuctionHouse.sol 355 verbs.burn(_auction.verbId); 359 verbs.burn(_auction.verbId);
Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation.
//f(x) is a low gas cost operation //g(y) is a high gas cost operation //Sort operations with different gas costs as follows f(x) || g(y) f(x) && g(y)
In the MaxHeap.maxHeapify function, the first if condition pos >= (size / 2) consumes more gas compared to the condition pos <= size.
File: packages/revolution/src/MaxHeap.sol 102 if (pos >= (size / 2) && pos <= size) return;
MaxHeap.maxHeapify to prioritize pos <= size over pos >= (size / 2) allows for efficient short-circuiting. If the low gas cost operation is true, the evaluation of the high gas cost operation can be bypassed. This optimization helps reduce gas consumption during execution.- if (pos >= (size / 2) && pos <= size) return; + if (pos <= size && pos >= (size / 2)) return;
This means that if you use uint8, EVM has to first convert it uint256 to work on it and the conversion costs extra gas! You may wonder, What were the devs thinking? Why did they create smaller variables then? The answer lies in packing. In solidity, you can pack multiple small variables into one slot, but if you are defining a lone variable and can’t pack it, it’s optimal to use a uint256 rather than uint8.
File: packages/revolution/src/AuctionHouse.sol 63 uint8 public minBidIncrementPercentage;
While uint8 variables occupy less storage space, the Ethereum Virtual Machine (EVM) requires additional gas to convert them to uint256 for performing operations. This conversion process incurs extra gas costs, which can be avoided by using uint256 directly
The reason for this is that constant variables are evaluated at runtime and their value is included in the bytecode of the contract. This means that any expensive operations performed as part of the constant expression, such as a call to keccak256(), will be executed every time the contract is deployed, even if the result is always the same. This can result in higher gas costs.
There are 1 instances of this issue:
File: packages/revolution/src/CultureIndex.sol 29 bytes32 public constant VOTE_TYPEHASH = 30 keccak256("Vote(address from,uint256[] pieceIds,uint256 nonce,uint256 deadline)");
Initializing a storage variable is one of the most expensive operations a contract can do.
When a storage variable goes from zero to non-zero, the user must pay 22,100 gas total (20,000 gas for a zero to non-zero write and 2,100 for a cold storage access).
This is why the Openzeppelin reentrancy guard registers functions as active or not with 1 and 2 rather than 0 and 1. It only costs 5,000 gas to alter a storage variable from non-zero to non-zero.
size is storage variableFile: packages/revolution/src/MaxHeap.sol 67 uint256 public size = 0;
If a similar external call is performed back-to-back, we can use assembly to reuse any function signatures and function parameters that stay the same. In addition, we can also reuse the same memory space for each function call (scratch space + free memory pointer + zero slot), which can potentially allow us to avoid memory expansion costs. Note: In order to do this optimization safely we will cache the free memory pointer value and restore it once we are done with our function calls. We will also set the zero slot back to 0 if neccessary.
check this report for implementation Reffrence
File: packages/revolution/src/AuctionHouse.sol 370 uint256 numCreators = verbs.getArtPieceById(_auction.verbId).creators.length; 371 address deployer = verbs.getArtPieceById(_auction.verbId).sponsor;
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
File: packages/revolution/src/CultureIndex.sol 30 keccak256("Vote(address from,uint256[] pieceIds,uint256 nonce,uint256 deadline)");
it's recommended to use the selfbalance() function instead of address(this).balance. The selfbalance() function is a built-in Solidity function that returns the balance of the current contract in Wei and is considered more gas-efficient and secure.
File: packages/revolution/src/AuctionHouse.sol 348 if (address(this).balance < reservePrice) { 421 if (address(this).balance < _amount) revert("Insufficient balance");
Performing STATICCALLs that do not depend on variables incremented in loops should always try to be avoided within the loop. In the following instances, we are able to cache the external calls outside of the loop to save a STATICCALL (100 gas) per loop iteration.
verbs.getArtPieceById(_auction.verbId) is external call cache this outside of loop then use to save gasFile: packages/revolution/src/AuctionHouse.sol 284 for (uint256 i = 0; i < numCreators; i++) { 385 ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
In Solidity, new bytes(0) and "" both represent an empty byte array. However, "" is more gas-efficient than new bytes(0) This is because "" is a literal and is stored in the code, whereas new bytes(0) is a dynamic allocation that requires the creation of a new memory location at runtime
File: packages/revolution/src/ERC20TokenEmitter.sol 191 (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
- 191 (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); + 191 (bool success, ) = treasury.call{ value: toPayTreasury }(""); - 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0)); + 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }("");
When calling an external function without specifying a gas limit , the called contract may consume all the remaining gas, causing the tx to be reverted. To mitigate this, it is recommended to explicitly set a gas limit when making low level external calls.
File: packages/revolution/src/ERC20TokenEmitter.sol 191 (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
- 191 (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); + 191 (bool success, ) = treasury.call{ value: toPayTreasury, gas: 50000 }(new bytes(0)); - 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0)); + 196 (success, ) = creatorsAddress.call{ value: creatorDirectPayment, gas: 50000 }(new bytes(0));
newPiece.totalVotesSupply instead of accessing it more than onceFile: packages/revolution/src/CultureIndex.sol 234 newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; 240 emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply);
In Solidity, strings are variable length dynamic data types, meaning their length can change and grow as needed.
If the length is 32 bytes or longer, the slot in which they are defined stores the length of the string * 2 + 1, while their actual data is stored elsewhere (the keccak hash of that slot).
However, if a string is less than 32 bytes, the length * 2 is stored at the least significant byte of it’s storage slot and the actual data of the string is stored starting from the most significant byte in the slot in which it is defined.
for more details check this
File: packages/revolution/src/CultureIndex.sol 57 string public name;
Similar to above, you can “just transfer” ether to a contract and have it respond to the transfer instead of using a payable function. This of course, depends on the rest of the contract’s architecture.
Example Deposit in AAVE
contract AddLiquidity{ receive() external payable { IWETH(weth).deposit{msg.value}(); AAVE.deposit(weth, msg.value, msg.sender, REFERRAL_CODE) } }
The fallback function is capable of receiving bytes data which can be parsed with abi.decode. This servers as an alternative to supplying arguments to a deposit function.
File: packages/revolution/src/AuctionHouse.sol 435 IWETH(WETH).deposit{ value: _amount }();
When reverting in solidity code, it is common practice to use a require or revert statement to revert execution with an error message. This can in most cases be further optimized by using assembly to revert with the error message.
Here’s an example;
/// calling restrictedAction(2) with a non-owner address: 24042 contract SolidityRevert { address owner; uint256 specialNumber = 1; constructor() { owner = msg.sender; } function restrictedAction(uint256 num) external { require(owner == msg.sender, "caller is not owner"); specialNumber = num; } } /// calling restrictedAction(2) with a non-owner address: 23734 contract AssemblyRevert { address owner; uint256 specialNumber = 1; constructor() { owner = msg.sender; } function restrictedAction(uint256 num) external { assembly { if sub(caller(), sload(owner.slot)) { mstore(0x00, 0x20) // store offset to where length of revert message is stored mstore(0x20, 0x13) // store length (19) mstore(0x40, 0x63616c6c6572206973206e6f74206f776e657200000000000000000000000000) // store hex representation of message revert(0x00, 0x60) // revert with data } } specialNumber = num; } }
From the example above we can see that we get a gas saving of over 300 gas when reverting wth the same error message with assembly against doing so in solidity. This gas savings come from the memory expansion costs and extra type checks the solidity compiler does under the hood.
There are 78 instances of this issue:
File: packages/protocol-rewards/src/abstract/RewardSplits.sol 30 if (_protocolRewards == address(0) || _revolutionRewardRecipient == address(0)) revert("Invalid Address Zero");
File: packages/revolution/src/AuctionHouse.sol 120 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/AuctionHouse.sol 121 require(_weth != address(0), "WETH cannot be zero address");
File: packages/revolution/src/AuctionHouse.sol 129 require( 130 _auctionParams.creatorRateBps >= _auctionParams.minCreatorRateBps, 131 "Creator rate must be greater than or equal to the creator rate" 132 );
File: packages/revolution/src/AuctionHouse.sol 175 require(bidder != address(0), "Bidder cannot be zero address");
File: packages/revolution/src/AuctionHouse.sol 176 require(_auction.verbId == verbId, "Verb not up for auction");
File: packages/revolution/src/AuctionHouse.sol 178 require(block.timestamp < _auction.endTime, "Auction expired");
File: packages/revolution/src/AuctionHouse.sol 179 require(msg.value >= reservePrice, "Must send at least reservePrice");
File: packages/revolution/src/AuctionHouse.sol 180 require( 181 msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), 182 "Must send more than last bid by minBidIncrementPercentage amount" 183 );
File: packages/revolution/src/AuctionHouse.sol 218 require( 219 _creatorRateBps >= minCreatorRateBps, 220 "Creator rate must be greater than or equal to minCreatorRateBps" 221 );
File: packages/revolution/src/AuctionHouse.sol 222 require(_creatorRateBps <= 10_000, "Creator rate must be less than or equal to 10_000");
File: packages/revolution/src/AuctionHouse.sol 234 require(_minCreatorRateBps <= creatorRateBps, "Min creator rate must be less than or equal to creator rate");
File: packages/revolution/src/AuctionHouse.sol 235 require(_minCreatorRateBps <= 10_000, "Min creator rate must be less than or equal to 10_000");
File: packages/revolution/src/AuctionHouse.sol 238 require( 239 _minCreatorRateBps > minCreatorRateBps, 240 "Min creator rate must be greater than previous minCreatorRateBps" 241 );
File: packages/revolution/src/AuctionHouse.sol 254 require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");
File: packages/revolution/src/AuctionHouse.sol 311 require(gasleft() >= MIN_TOKEN_MINT_GAS_THRESHOLD, "Insufficient gas for creating auction");
File: packages/revolution/src/AuctionHouse.sol 339 require(_auction.startTime != 0, "Auction hasn't begun");
File: packages/revolution/src/AuctionHouse.sol 340 require(!_auction.settled, "Auction has already been settled");
File: packages/revolution/src/AuctionHouse.sol 342 require(block.timestamp >= _auction.endTime, "Auction hasn't completed");
File: packages/revolution/src/AuctionHouse.sol 421 if (address(this).balance < _amount) revert("Insufficient balance");
File: packages/revolution/src/AuctionHouse.sol 441 if (!wethSuccess) revert("WETH transfer failed");
File: packages/revolution/src/CultureIndex.sol 117 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/CultureIndex.sol 119 require(_cultureIndexParams.quorumVotesBPS <= MAX_QUORUM_VOTES_BPS, "invalid quorum bps");
File: packages/revolution/src/CultureIndex.sol 120 require(_cultureIndexParams.erc721VotingTokenWeight > 0, "invalid erc721 voting token weight");
File: packages/revolution/src/CultureIndex.sol 121 require(_erc721VotingToken != address(0), "invalid erc721 voting token");
File: packages/revolution/src/CultureIndex.sol 122 require(_erc20VotingToken != address(0), "invalid erc20 voting token");
File: packages/revolution/src/CultureIndex.sol 160 require(uint8(metadata.mediaType) > 0 && uint8(metadata.mediaType) <= 5, "Invalid media type");
File: packages/revolution/src/CultureIndex.sol 163 require(bytes(metadata.image).length > 0, "Image URL must be provided");
File: packages/revolution/src/CultureIndex.sol 165 require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided");
File: packages/revolution/src/CultureIndex.sol 167 require(bytes(metadata.text).length > 0, "Text must be provided");
File: packages/revolution/src/CultureIndex.sol 182 require(creatorArrayLength <= MAX_NUM_CREATORS, "Creator array must not be > MAX_NUM_CREATORS");
File: packages/revolution/src/CultureIndex.sol 186 require(creatorArray[i].creator != address(0), "Invalid creator address");
File: packages/revolution/src/CultureIndex.sol 190 require(totalBps == 10_000, "Total BPS must sum up to 10,000");
File: packages/revolution/src/CultureIndex.sol 308 require(pieceId < _currentPieceId, "Invalid piece ID");
File: packages/revolution/src/CultureIndex.sol 309 require(voter != address(0), "Invalid voter address");
File: packages/revolution/src/CultureIndex.sol 310 require(!pieces[pieceId].isDropped, "Piece has already been dropped");
File: packages/revolution/src/CultureIndex.sol 311 require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");
File: packages/revolution/src/CultureIndex.sol 314 require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");
File: packages/revolution/src/CultureIndex.sol 398 require( 399 len == pieceIds.length && len == deadline.length && len == v.length && len == r.length && len == s.length, 400 "Array lengths must match" 401 );
File: packages/revolution/src/CultureIndex.sol 427 require(deadline >= block.timestamp, "Signature expired");
File: packages/revolution/src/CultureIndex.sol 452 require(pieceId < _currentPieceId, "Invalid piece ID");
File: packages/revolution/src/CultureIndex.sol 462 require(pieceId < _currentPieceId, "Invalid piece ID");
File: packages/revolution/src/CultureIndex.sol 487 require(maxHeap.size() > 0, "Culture index is empty");
File: packages/revolution/src/CultureIndex.sol 499 require(newQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS, "CultureIndex::_setQuorumVotesBPS: invalid quorum bps");
File: packages/revolution/src/CultureIndex.sol 520 require(msg.sender == dropperAdmin, "Only dropper can drop pieces");
File: packages/revolution/src/CultureIndex.sol 523 require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped.");
File: packages/revolution/src/ERC20TokenEmitter.sol 91 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/ERC20TokenEmitter.sol 96 require(_treasury != address(0), "Invalid treasury address");
File: packages/revolution/src/ERC20TokenEmitter.sol 158 require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");
File: packages/revolution/src/ERC20TokenEmitter.sol 160 require(msg.value > 0, "Must send ether");
File: packages/revolution/src/ERC20TokenEmitter.sol 162 require(addresses.length == basisPointSplits.length, "Parallel arrays required");
File: packages/revolution/src/ERC20TokenEmitter.sol 192 require(success, "Transfer failed.");
File: packages/revolution/src/ERC20TokenEmitter.sol 197 require(success, "Transfer failed.");
File: packages/revolution/src/ERC20TokenEmitter.sol 217 require(bpsSum == 10_000, "bps must add up to 10_000");
File: packages/revolution/src/ERC20TokenEmitter.sol 238 require(amount > 0, "Amount must be greater than 0");
File: packages/revolution/src/ERC20TokenEmitter.sol 255 require(etherAmount > 0, "Ether amount must be greater than 0");
File: packages/revolution/src/ERC20TokenEmitter.sol 272 require(paymentAmount > 0, "Payment amount must be greater than 0");
File: packages/revolution/src/ERC20TokenEmitter.sol 289 require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");
File: packages/revolution/src/ERC20TokenEmitter.sol 300 require(_creatorRateBps <= 10_000, "Creator rate must be less than or equal to 10_000");
File: packages/revolution/src/ERC20TokenEmitter.sol 310 require(_creatorsAddress != address(0), "Invalid address");
File: packages/revolution/src/MaxHeap.sol 42 require(msg.sender == admin, "Sender is not the admin");
File: packages/revolution/src/MaxHeap.sol 56 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/MaxHeap.sol 79 require(pos != 0, "Position should not be zero");
File: packages/revolution/src/MaxHeap.sol 157 require(size > 0, "Heap is empty");
File: packages/revolution/src/MaxHeap.sol 170 require(size > 0, "Heap is empty");
File: packages/revolution/src/NontransferableERC20Votes.sol 69 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/VerbsToken.sol 76 require(!isMinterLocked, "Minter is locked");
File: packages/revolution/src/VerbsToken.sol 84 require(!isCultureIndexLocked, "CultureIndex is locked");
File: packages/revolution/src/VerbsToken.sol 92 require(!isDescriptorLocked, "Descriptor is locked");
File: packages/revolution/src/VerbsToken.sol 100 require(msg.sender == minter, "Sender is not the minter");
File: packages/revolution/src/VerbsToken.sol 137 require(msg.sender == address(manager), "Only manager can initialize");
File: packages/revolution/src/VerbsToken.sol 139 require(_minter != address(0), "Minter cannot be zero address");
File: packages/revolution/src/VerbsToken.sol 140 require(_initialOwner != address(0), "Initial owner cannot be zero address");
File: packages/revolution/src/VerbsToken.sol 210 require(_minter != address(0), "Minter cannot be zero address");
File: packages/revolution/src/VerbsToken.sol 274 require(verbId <= _currentVerbId, "Invalid piece ID");
File: packages/revolution/src/VerbsToken.sol 286 require( 287 artPiece.creators.length <= cultureIndex.MAX_NUM_CREATORS(), 288 "Creator array must not be > MAX_NUM_CREATORS" 289 );
File: packages/revolution/src/VerbsToken.sol 317 revert("dropTopVotedPiece failed");
330 require(manager.isRegisteredUpgrade(_getImplementation(), _newImpl), "Invalid upgrade");
Similar to splitting require statements, you will usually save some gas by not having a boolean operator in the if statement.
contract CustomErrorBoolLessEfficient { error BadValue(); function requireGood(uint256 x) external pure { if (x < 10 || x > 20) { revert BadValue(); } } } contract CustomErrorBoolEfficient { error TooLow(); error TooHigh(); function requireGood(uint256 x) external pure { if (x < 10) { revert TooLow(); } if (x > 20) { revert TooHigh(); } } }
File: packages/revolution/src/CultureIndex.sol 441 if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE();
41 if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT();
Although the calldataload instruction is a cheap opcode, the solidity compiler will sometimes output cheaper code if you cache calldataload. This will not always be the case, so you should test both possibilities.
contract LoopSum { function sumArr(uint256[] calldata arr) public pure returns (uint256 sum) { uint256 len = arr.length; for (uint256 i = 0; i < len; ) { sum += arr[i]; unchecked { ++i; } } } }
File: packages/revolution/src/ERC20TokenEmitter.sol 153 address[] calldata addresses,
cachedAddresses variable of type address[] memory to cache the entire addresses array. The cachedAddresses array can be accessed and iterated over later in buyToken function using a loop or any other desired logic.function buyToken( address[] calldata addresses, uint[] calldata basisPointSplits, ProtocolRewardAddresses calldata protocolRewardsRecipients ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) { //prevent treasury from paying itself require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens"); require(msg.value > 0, "Must send ether"); + // Cache the addresses array + address[] memory cachedAddresses = addresses; // ensure the same number of addresses and bps - require(addresses.length == basisPointSplits.length, "Parallel arrays required"); + require(cachedAddresses.length == basisPointSplits.length, "Parallel arrays required"); // Get value left after protocol rewards uint256 msgValueRemaining = _handleRewardsAndGetValueToSend( msg.value, protocolRewardsRecipients.builder, protocolRewardsRecipients.purchaseReferral, protocolRewardsRecipients.deployer ); //Share of purchase amount to send to treasury uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000; //Share of purchase amount to reserve for creators //Ether directly sent to creators uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000; //Tokens to emit to creators int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0 ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment) : int(0); // Tokens to emit to buyers int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0); //Transfer ETH to treasury and update emitted emittedTokenWad += totalTokensForBuyers; if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators; //Deposit funds to treasury (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); require(success, "Transfer failed."); //Transfer ETH to creators if (creatorDirectPayment > 0) { (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0)); require(success, "Transfer failed."); } //Mint tokens for creators if (totalTokensForCreators > 0 && creatorsAddress != address(0)) { _mint(creatorsAddress, uint256(totalTokensForCreators)); } uint256 bpsSum = 0; //Mint tokens to buyers - for (uint256 i = 0; i < addresses.length; i++) { - for (uint256 i = 0; i < cachedAddresses.length; i++) { if (totalTokensForBuyers > 0) { // transfer tokens to address - _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000)); + _mint(cachedAddresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000)); } bpsSum += basisPointSplits[i]; } require(bpsSum == 10_000, "bps must add up to 10_000"); emit PurchaseFinalized( msg.sender, msg.value, toPayTreasury, msg.value - msgValueRemaining, uint256(totalTokensForBuyers), uint256(totalTokensForCreators), creatorDirectPayment ); return uint256(totalTokensForBuyers); }
File: packages/revolution/src/CultureIndex.sol 433 bytes32 digest = _hashTypedDataV4(voteHash);
function _verifyVoteSignature( address from, uint256[] calldata pieceIds, uint256 deadline, uint8 v, bytes32 r, bytes32 s ) internal returns (bool success) { require(deadline >= block.timestamp, "Signature expired"); bytes32 voteHash; voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline)); - bytes32 digest = _hashTypedDataV4(voteHash); - address recoveredAddress = ecrecover(digest, v, r, s); + address recoveredAddress = ecrecover(_hashTypedDataV4(voteHash), v, r, s); // Ensure to address is not 0 if (from == address(0)) revert ADDRESS_ZERO(); // Ensure signature is valid if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE(); return true; }
it's generally a good practice to limit the visibility of state variables to the minimum necessary level. This means that you should use the private visibility modifier for state variables whenever possible, and avoid using the public modifier unless it is absolutely necessary.
1.Security: Public state variables can be read and modified by anyone on the blockchain, which can make your contract vulnerable to attacks. By using the private modifier, you can limit access to your state variables and reduce the risk of malicious actors exploiting them.
2.Encapsulation: Using private state variables can help to encapsulate the internal workings of your contract and make it easier to reason about and maintain. By only exposing the necessary interfaces to the outside world, you can reduce the complexity and potential for errors in your contract.
3.Gas costs: Public state variables can be more expensive to read and write than private state variables, since Solidity generates additional getter and setter functions for public variables. By using private state variables, you can reduce the gas cost of your contract and improve its efficiency.
If you do not require other contracts to read these variables, consider making them private or internal.
File: packages/revolution/src/CultureIndex.sol 78 address public dropperAdmin;
File: packages/revolution/src/MaxHeap.sol 16 address public admin;
File: packages/revolution/src/VerbsToken.sol 42 address public minter;
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past.
File: packages/revolution/src/VerbsToken.sol 221 isMinterLocked = true; 243 isDescriptorLocked = true; 263 isCultureIndexLocked = true;
Modifiers inject its implementation bytecode where it is used while internal functions jump to the location in the runtime code where the its implementation is. This brings certain trade-offs to both options.
Using modifiers more than once means repetitiveness and increase in size of the runtime code but reduces gas cost because of the absence of jumping to the internal function execution offset and jumping back to continue. This means that if runtime gas cost matter most to you, then modifiers should be your choice but if deployment gas cost and/or reducing the size of the creation code is most important to you then using internal functions will be best.
However, modifiers have the tradeoff that they can only be executed at the start or end of a functon. This means executing it at the middle of a function wouldn’t be directly possible, at least not without internal functions which kill the original purpose. This affects it’s flexibility. Internal functions however can be called at any point in a function.
Example showing difference in gas cost using modifiers and an internal function
// SPDX-License-Identifier: MIT pragma solidity 0.8.19; /** deployment gas cost: 195435 gas per call: restrictedAction1: 28367 restrictedAction2: 28377 restrictedAction3: 28411 */ contract Modifier { address owner; uint256 val; constructor() { owner = msg.sender; } modifier onlyOwner() { require(msg.sender == owner); _; } function restrictedAction1() external onlyOwner { val = 1; } function restrictedAction2() external onlyOwner { val = 2; } function restrictedAction3() external onlyOwner { val = 3; } } /** deployment gas cost: 159309 gas per call: restrictedAction1: 28391 restrictedAction2: 28401 restrictedAction3: 28435 */ contract InternalFunction { address owner; uint256 val; constructor() { owner = msg.sender; } function onlyOwner() internal view { require(msg.sender == owner); } function restrictedAction1() external { onlyOwner(); val = 1; } function restrictedAction2() external { onlyOwner(); val = 2; } function restrictedAction3() external { onlyOwner(); val = 3; } }
| Operation | Deployment | restrictedAction1 | restrictedAction2 | restrictedAction3 |
|---|---|---|---|---|
| Modifiers | 195435 | 28367 | 28377 | 28411 |
| Internal Functions | 159309 | 28391 | 28401 | 28435 |
File: packages/revolution/src/VerbsToken.sol modifier whenMinterNotLocked() { require(!isMinterLocked, "Minter is locked"); _; } /** * @notice Require that the CultureIndex has not been locked. */ modifier whenCultureIndexNotLocked() { require(!isCultureIndexLocked, "CultureIndex is locked"); _; } /** * @notice Require that the descriptor has not been locked. */ modifier whenDescriptorNotLocked() { require(!isDescriptorLocked, "Descriptor is locked"); _; } /** * @notice Require that the sender is the minter. */ modifier onlyMinter() { require(msg.sender == minter, "Sender is not the minter"); _; }
If needed, 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
Using public for immutable variable gas value: 704034 Using private for immutable variable gas value: 685260
int256 public immutable targetPrice; int256 public immutable perTimeUnit; int256 public immutable decayConstant; int256 public immutable priceDecayPercent;
We can avoid unnecessary offset calculations by moving the storage pointer to the top of the function.
File: packages/revolution/src/CultureIndex.sol 223 ArtPiece storage newPiece = pieces[pieceId];
function createPiece( ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray ) public returns (uint256) { + uint256 pieceId = _currentPieceId++; + ArtPiece storage newPiece = pieces[pieceId]; uint256 creatorArrayLength = validateCreatorsArray(creatorArray); // Validate the media type and associated data validateMediaType(metadata); - uint256 pieceId = _currentPieceId++; /// @dev Insert the new piece into the max heap maxHeap.insert(pieceId, 0); - ArtPiece storage newPiece = pieces[pieceId]; newPiece.pieceId = pieceId; newPiece.totalVotesSupply = _calculateVoteWeight( erc20VotingToken.totalSupply(), erc721VotingToken.totalSupply() ); newPiece.totalERC20Supply = erc20VotingToken.totalSupply(); newPiece.metadata = metadata; newPiece.sponsor = msg.sender; newPiece.creationBlock = block.number; newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; for (uint i; i < creatorArrayLength; i++) { newPiece.creators.push(creatorArray[i]); } emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply); // Emit an event for each creator for (uint i; i < creatorArrayLength; i++) { emit PieceCreatorAdded(pieceId, creatorArray[i].creator, msg.sender, creatorArray[i].bps); } return newPiece.pieceId; }
Use assembly for math instead of Solidity. You can check for overflow/underflow in assembly to ensure safety.
Addition:
//addition in SolidityCache multiple accesses of a mapping/array function addTest(uint256 a, uint256 b) public pure { uint256 c = a + b; }
Gas: 303
//addition in assembly function addAssemblyTest(uint256 a, uint256 b) public pure { assembly { let c := add(a, b) if lt(c, a) { mstore(0x00, "overflow") revert(0x00, 0x20) } } }
Gas: 263
Subtraction
//subtraction in Solidity function subTest(uint256 a, uint256 b) public pure { uint256 c = a - b; }
Gas: 300
//subtraction in assembly function subAssemblyTest(uint256 a, uint256 b) public pure { assembly { let c := sub(a, b) if gt(c, a) { mstore(0x00, "underflow") revert(0x00, 0x20) } } }
Gas: 263
Multiplication
//multiplication in Solidity function mulTest(uint256 a, uint256 b) public pure { uint256 c = a * b; }
Gas: 325
//multiplication in assembly function mulAssemblyTest(uint256 a, uint256 b) public pure { assembly { let c := mul(a, b) if lt(c, a) { mstore(0x00, "overflow") revert(0x00, 0x20) } } }
Gas: 265
Division
//division in Solidity function divTest(uint256 a, uint256 b) public pure { uint256 c = a * b; }
Gas: 325
//division in assembly function divAssemblyTest(uint256 a, uint256 b) public pure { assembly { let c := div(a, b) if gt(c, a) { mstore(0x00, "underflow") revert(0x00, 0x20) } } }
Gas: 265
File: packages/revolution/src/CultureIndex.sol 285 return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18);
function _calculateVoteWeight(uint256 erc20Balance, uint256 erc721Balance) internal view returns (uint256) { - return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18); + uint256 voteWeight; + assembly { + // Calculate erc721Balance * erc721VotingTokenWeight in assembly + let result := mul(erc721Balance, erc721VotingTokenWeight) + // Multiply the result by 1e18 (10^18) in assembly + voteWeight := mul(result, 1e18) + // Add erc20Balance to voteWeight + voteWeight := add(voteWeight, erc20Balance) + } + return voteWeight; }
fixes provided aim to prevent zero amounts from being passed to the mint() and _mint() functions in the ERC20TokenEmitter.sol and NontransferableERC20Votes.sol contracts, respectively. By adding a condition to check if the amount is greater than zero before executing the minting operation, gas savings can be achieved by avoiding unnecessary function calls with zero amounts.
File: packages/revolution/src/ERC20TokenEmitter.sol 109 token.mint(_to, _amount);
function _mint(address _to, uint256 _amount) private { - token.mint(_to, _amount); + if (amount > 0){ + token.mint(_to, _amount); + } }
File: packages/revolution/src/NontransferableERC20Votes.sol 135 _mint(account, amount);
function mint(address account, uint256 amount) public onlyOwner { - _mint(account, amount); + if (amount > 0){ + _mint(account, amount); + } }
80 protocolRewards.depositRewards{ value: totalReward }(
function _depositPurchaseRewards( uint256 paymentAmountWei, address builderReferral, address purchaseReferral, address deployer ) internal returns (uint256) { (RewardsSettings memory settings, uint256 totalReward) = computePurchaseRewards(paymentAmountWei); if (builderReferral == address(0)) builderReferral = revolutionRewardRecipient; if (deployer == address(0)) deployer = revolutionRewardRecipient; if (purchaseReferral == address(0)) purchaseReferral = revolutionRewardRecipient; + if (totalReward > 0) { protocolRewards.depositRewards{ value: totalReward }( builderReferral, settings.builderReferralReward, purchaseReferral, settings.purchaseReferralReward, deployer, settings.deployerReward, revolutionRewardRecipient, settings.revolutionReward ); + } return totalReward; }
_amount == 0 is added to the first if statement. If the _amount is zero, the function will revert with the error message "Invalid amount." This prevents the transfer from being executed when the amount is zero, saving gas and avoiding unnecessary operations.File: packages/revolution/src/AuctionHouse.sol function _safeTransferETHWithFallback(address _to, uint256 _amount) private { // Ensure the contract has enough ETH to transfer - if (address(this).balance < _amount) revert("Insufficient balance"); + if (address(this).balance < _amount || _amount == 0) revert("Invalid amount"); // Used to store if the transfer succeeded bool success; assembly { // Transfer ETH to the recipient // Limit the call to 50,000 gas success := call(50000, _to, _amount, 0, 0, 0, 0) } // If the transfer failed: if (!success) { // Wrap as WETH IWETH(WETH).deposit{ value: _amount }(); // Transfer WETH instead bool wethSuccess = IWETH(WETH).transfer(_to, _amount); // Ensure successful transfer if (!wethSuccess) revert("WETH transfer failed"); } }
Modifiers code is copied in all instances where it's used, increasing bytecode size. By doing a refractor to the internal function, one can reduce bytecode size significantly at the cost of one JUMP. Consider doing this only if you are constrained by bytecode size.
Before:
modifier onlyOwner() { require(owner() == msg.sender, "Ownable: caller is not the owner"); _; }
After:
modifier onlyOwner() { _checkOwner(); _; } function _checkOwner() internal view virtual { require(owner() == msg.sender, "Ownable: caller is not the owner"); }
File: packages/revolution/src/VerbsToken.sol 75 modifier whenMinterNotLocked() { require(!isMinterLocked, "Minter is locked"); _; } /** * @notice Require that the CultureIndex has not been locked. */ 83 modifier whenCultureIndexNotLocked() { require(!isCultureIndexLocked, "CultureIndex is locked"); _; } /** * @notice Require that the descriptor has not been locked. */ 91 modifier whenDescriptorNotLocked() { require(!isDescriptorLocked, "Descriptor is locked"); _; } /** * @notice Require that the sender is the minter. */ 99 modifier onlyMinter() { require(msg.sender == minter, "Sender is not the minter"); _; }
File: packages/revolution/src/MaxHeap.sol 41 modifier onlyAdmin() { require(msg.sender == admin, "Sender is not the admin"); _; }
#0 - c4-pre-sort
2023-12-24T01:45:19Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2024-01-07T13:34:01Z
MarioPoneder marked the issue as grade-b
🌟 Selected for report: pavankv
Also found by: 0xAsen, ABAIKUNANBAEV, Raihan, Sathish9098, ZanyBonzy, albahaca, hunter_w3b, ihtishamsudo, kaveyjoe, peanuts, unique, wahedtalash77
25.6332 USDC - $25.63
Revolution is a set of contracts that improve on <ins>Nouns DAO</ins>. Nouns is a generative avatar collective that auctions off one ERC721, every day, forever. 100% of the proceeds of each auction (the winning bid) go into a shared treasury, and owning an NFT gets you 1 vote over the treasury.
The ultimate goal of Revolution is fair ownership distribution over a community movement where anyone can earn decision making power over the energy of the movement. If this excites you, <ins>build with us</ins>.
1. Initial Observations:
2. Security Analysis:
3. Best Practices:
4. Potential Issues:
5. Recommendations:
6. Architecture Recommendations:
7. Codebase Quality Analysis:
8. Centralization Risks:
9. Mechanism Review:
10. Systemic Risks: - Underlined the significance of a comprehensive security audit for identifying and mitigating systemic risks. - Emphasized the need for ongoing testing, monitoring, and careful consideration of potential issues.
MaxHeap.sol
initialize function to prevent re-initialization.initializer modifier on the constructor for clarity and conventional usage.maxHeapify function to prevent out-of-bounds access.CultureIndex.sol
createPiece function to prevent spam or misuse, such as rate limiting or permissions.NontransferableERC20Votes.sol
initialize function to ensure that the initialization is secure and resistant to potential vulnerabilities in the manager contract.ERC20TokenEmitter.sol
block.timestamp and its potential limitations.manager address.basisPointSplits equals 10,000, consider adding individual checks to ensure each basis point value is valid.AuctionHouse.sol
VerbsToken.sol
descriptor, cultureIndex, manager) to users and auditors.lockMinter, lockDescriptor, lockCultureIndex). Ensure that this design aligns with the intended use and is well-communicated to users.VRGDAC.sol
TokenEmitterRewards.sol
Given the abstract nature of TokenEmitterRewards, it's crucial to review and ensure that child contracts implementing this abstract contract adhere to its requirements. Additionally, providing documentation or comments explaining the intended use of the contract and its functions would enhance understandability for developers.
RewardSplits.sol
IRevolutionProtocolRewards prevents reentrancy attacks. Implement the reentrancy guard in _depositPurchaseRewards._depositPurchaseRewards: Add input validation checks for payment amount and parameters.revolutionRewardRecipient: Assess centralization risk and alternative approaches.MaxHeap.sol
The codebase is well-structured, and the usage of upgradeable patterns and established libraries (OpenZeppelin) indicates a commitment to best practices. However, the unconventional use of initializer on the constructor and potential re-initialization issues could be improved for better code quality.
CultureIndex.sol
The usage of OpenZeppelin contracts and established patterns such as UUPS indicates a commitment to best practices. The contract is feature-rich, but the complexity also warrants careful scrutiny during audits to ensure correctness and security.
NontransferableERC20Votes.sol
The use of OpenZeppelin contracts and upgradeability patterns indicates adherence to best practices. The code is concise and focused, catering to the specific requirements of a nontransferable ERC-20 with voting features.
ERC20TokenEmitter.sol
The contract leverages OpenZeppelin contracts and follows best practices for upgradeability, pausability, and ownership transfer. The use of modifiers, such as nonReentrant, enhances security. The contract structure appears well-organized, facilitating readability.
AuctionHouse.sol
VerbsToken.sol
The contract leverages OpenZeppelin contracts, including reentrancy guards and upgradeability patterns, indicating a commitment to best practices. The use of modifiers enhances security, and the code structure appears organized.
VRGDAC.sol
SignedWadMath.sol). If possible, use well-established and audited libraries to minimize risks.TokenEmitterRewards.sol
The codebase demonstrates adherence to good practices such as specifying the license, version, and using custom errors for clarity. However, the quality assessment would be more comprehensive with access to the full codebase, especially the implementation of the RewardSplits contract.
MaxHeap.sol
The onlyAdmin modifier might introduce centralization risks, as critical functions are restricted to a specific address (admin). Depending on the intended design, it's important to carefully manage and secure the admin role to prevent potential misuse or compromise.
CultureIndex.sol
The reliance on a trusted manager for critical functions like initialization and upgrades introduces centralization risks. It's crucial to ensure that the manager contract is secure and well-protected to prevent potential exploits.
NontransferableERC20Votes.sol
The reliance on the manager address for initialization introduces centralization risks. It's essential to ensure the security and trustworthiness of the manager contract, considering its pivotal role in the contract's lifecycle.
ERC20TokenEmitter.sol
Ensuring the trustworthiness and security of the manager address is crucial, given its role in contract initialization. Clear documentation on the expectations and responsibilities of the manager address would add transparency.
AuctionHouse.sol
The contract's manager, which controls upgrades, is set as immutable, reducing centralization risks. However, the upgrade process should be managed and audited thoroughly.
VerbsToken.sol
The reliance on external contracts (descriptor, cultureIndex, manager) introduces centralization risks. It is crucial to ensure the security and trustworthiness of these external contracts, as their behavior can impact the overall security of the VerbsToken contract.
VRGDAC.sol , TokenEmitterRewards.sol
No centralization risks
MaxHeap.sol
The use of the UUPS pattern for upgradeability is a positive aspect, enhancing the contract's flexibility and upgradability. The upgrade management functions, such as _authorizeUpgrade, contribute to a structured upgrade process.
CultureIndex.sol
The voting mechanisms, gasless voting with signatures, and art piece management are well-implemented, providing flexibility and user-friendly interactions. The use of nonces for preventing replay attacks in gasless voting is a good security measure.
NontransferableERC20Votes.sol
The contract's approach to nontransferability aligns with its specific use case, providing a clear and focused implementation. The minting functionality is well-documented but could benefit from additional transparency measures.
ERC20TokenEmitter.sol
The contract efficiently integrates a Variable Rate Gradual Dutch Auction Contract (VRGDAC) for token pricing, adding flexibility and dynamic pricing based on market conditions. The contract management functions provide control over critical parameters.
AuctionHouse.sol
The contract uses a secure UUPS pattern for upgrades, and the _authorizeUpgrade function ensures that only the owner can upgrade when the contract is paused. The use of safeTransferETHWithFallback is a good practice for fund transfer.
VerbsToken.sol
The use of upgradeability checks in _authorizeUpgrade adds a layer of security to the upgrade process. The locking mechanism for critical functionalities reflects a commitment to a certain design philosophy but should be carefully managed.
VRGDAC.sol
The contract implements a Continuous Variable Rate Gradual Dutch Auction (VRGDA), and the breakdown of parameters and functions helps in understanding its mechanism. No specific recommendations unless there are additional details or requirements for the mechanism.
TokenEmitterRewards.sol
The _handleRewardsAndGetValueToSend function appears to handle reward distribution logic. A thorough review of the RewardSplits contract is necessary to understand how rewards are distributed and whether there are potential vulnerabilities in this process.
MaxHeap.sol
The potential security flaws identified, if not addressed, could pose systemic risks. Specifically, re-initialization, unconventional constructor usage, and lack of bounds checking in heap operations could impact the overall security and functionality of the contract.
CultureIndex.sol
It cover a wide range of potential risks, including overflow/underflow issues, spam/misuse prevention, and the secure management of the manager role. Addressing these concerns through careful code review and auditing is essential.
NontransferableERC20Votes.sol
potential security flaws address concerns related to the initialize function, token nontransferability, minting control, and reliance on the manager address. Ensuring proper checks and balances on the manager contract is crucial for security.
ERC20TokenEmitter.sol
It cover a wide range of potential risks, and the use of require statements, event emission, and the Ownable2StepUpgradeable pattern contribute to a robust security posture.
AuctionHouse.sol
Given the financial nature of the contract, a comprehensive security audit is essential to identify and mitigate potential systemic risks. Thorough testing, monitoring, and careful consideration of potential issues are crucial.
VerbsToken.sol
potential security flaws highlight concerns related to the trustworthiness of external contracts, locking mechanisms, and the upgrade process. A thorough audit of the entire system, including external contracts and the upgrade process, is crucial to ensure overall security.
VRGDAC.sol
SignedWadMath.sol) for potential updates or vulnerabilities.
15 hours
#0 - c4-pre-sort
2023-12-24T00:41:30Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2024-01-07T14:28:00Z
MarioPoneder marked the issue as grade-b