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: 34/110
Findings: 1
Award: $201.70
🌟 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
201.6999 USDC - $201.70
Number | Issue |
---|---|
[G-01] | State variables can be packed into fewer storage slot (saves 12000 Gas) |
[G-02] | Pack structs by putting variables that can fit together next to each other (saves 8000 Gas) |
[G-03] | Unnecessary modifier checks in constructors (saves 144000 GAS) |
[G-04] | Unnecessary modifier checks in functions(saves 123500 GAS) |
[G-05] | Remove Vote struct as value from votes mapping in place of that use only uin256 weight as value |
[G-06] | State variables should be cached in stack variables rather than re-reading them from storage |
[G-07] | State address variable which is fix for chain can be marked immutable and initialize in constructor |
[G-08] | Do not assign struct value second time if it is already assigned it saves multiple MLOAD and MSTORE |
[G-09] | Using storage instead of memory for structs/arrays saves gas (saves 4200 GAS) |
[G-10] | Refactor code to fail early |
[G-11] | Refactor the computeTotalReward function to save gas |
[G-12] | Refactor the _vote function to save gas |
[G-13] | Remove unnecessary checks |
[G-14] | Missing zero-address check in constructor |
SAVE: 12000 GAS, 6 SLOT
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper
creatorRateBps
, minCreatorRateBps
, entropyRateBps
and address WETH
can be packed in single SLOT :SAVES 6000 GAS
, 3 SLOT
creatorRateBps
, minCreatorRateBps
, and entropyRateBps
can't be more than 10000 because of these checks :
File : main/packages/revolution/src/AuctionHouse.sol 218: require( 219: _creatorRateBps >= minCreatorRateBps, 220: "Creator rate must be greater than or equal to minCreatorRateBps" 221: ); 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");
So uint16 is more than sufficient to hold creatorRateBps
, minCreatorRateBps
, and entropyRateBps
because these value can't more than 10000 and uint16 can easily hold up to 0 to 65535. All 3 Bps will take 6 bytes in slot so these can be packed with address because address take 20 byte space in 32 byte storage slot.
File : main/packages/revolution/src/AuctionHouse.sol 54: address public WETH; + uint16 public creatorRateBps; + uint16 public minCreatorRateBps; + uint16 public entropyRateBps; 55: 56: // The minimum amount of time left in an auction after a new bid is created 57: uint256 public timeBuffer; 58: 59: // The minimum price accepted in an auction 60: uint256 public reservePrice; 61: 62: // The minimum percentage difference between the last bid amount and the current bid 63: uint8 public minBidIncrementPercentage; 64: 65: // The split of the winning bid that is reserved for the creator of the Verb in basis points -66: uint256 public creatorRateBps; 67: 68: // The all time minimum split of the winning bid that is reserved for the creator of the Verb in basis points -69: uint256 public minCreatorRateBps; 70: 71: // The split of (auction proceeds * creatorRate) that is sent to the creator as ether in basis points -72: uint256 public entropyRateBps;
quorumVotesBPS
and dropperAdmin
can be packed to single SLOT :SAVES 2000 GAS, 1 SLOT
quorumVotesBPS
can't more than 6000 because of these checks:
File : main/packages/revolution/src/CultureIndex.sol 48: uint256 public constant MAX_QUORUM_VOTES_BPS = 6_000; // 6,000 basis points or 60% 498: function _setQuorumVotesBPS(uint256 newQuorumVotesBPS) external onlyOwner { 499: require(newQuorumVotesBPS <= MAX_QUORUM_VOTES_BPS, "CultureIndex::_setQuorumVotesBPS: invalid quorum bps");
Here quorumVotesBPS
can't more than MAX_QUORUM_VOTES_BPS and MAX_QUORUM_VOTES_BPS is 6000. So quorumVotesBPS
can be easily reduced to uint16 and pack with an address dropperAdmin
because address take up space of 20 bytes in a 32 byte storage slot. So quorumVotesBPS
can easily pack with dropperAdmin
.
File: packages/revolution/src/CultureIndex.sol -54: uint256 public quorumVotesBPS; ... 78: address public dropperAdmin; +54: uint16 public quorumVotesBPS;
creatorRateBps
, entropyRateBps
and creatorsAddress
can be packed in single SLOT :SAVES 4000 GAS, 2 SLOT
creatorRateBps
and entropyRateBps
can't more than 10000 because of these checks:
File : main/packages/revolution/src/ERC20TokenEmitter.sol 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");
So uint16 is more than sufficient to hold creatorRateBps
, entropyRateBps
because these value can't more than 10000 and uint16 can easily hold up to 0 to 65535. creatorRateBps
and entropyRateBps
can be packed with address because address take 20 byte space in 32 byte storage slot.
File : main/packages/revolution/src/ERC20TokenEmitter.sol -42: uint256 public creatorRateBps; +42: uint16 public creatorRateBps; -45: uint256 public entropyRateBps; +45: uint16 public entropyRateBps; 48: // The account or contract to pay the creator reward to address public creatorsAddress;
SAVE: 8000 GAS, 4 SLOT
Since interfaces directly not in scope but struct defined inside them used by in-scope contracts ie. variables defined in-scope contracts using those struct from interfaces. So these varaibles will take storage space inside in-scope. So to by packing struct in interfaces will save SLOTs and Gas inside in-scope contracts. So that's why I am including those interfaces struct packing which are inherited by in-scope contracts.
As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, this saves gas when writing to storage ~20000 gas
startTime
and endTime
to uint40 and pack with bidder
:SAVES 4000 GAS, 2 SLOT
The uint40 type is safe for approximately 1.099511627776e+12 years. The exact number of years that a uint40 type is safe for is 1099511627776 years.
File : main/packages/revolution/src/interfaces/IAuctionHouse.sol 23: struct Auction { // ID for the Verb (ERC721 token ID) uint256 verbId; // The current highest bid amount uint256 amount; // The time that the auction started - uint256 startTime; // The time that the auction is scheduled to end - uint256 endTime; // The address of the current highest bid address payable bidder; + uint40 startTime; + uint40 endTime; // Whether or not the auction has been settled bool settled; 36: }
bps
and creator
can be packed in single SLOT :SAVES 2000 GAS, 1 SLOT
Reduce uint type for bps to uint16
because bps can't more than 10000 so uint16
is enough to hold it.
File : main/packages/revolution/src/interfaces/ICultureIndex.sol 98: struct CreatorBps { 99: address creator; +100: uint16 bps; -100: uint256 bps; 101: }
creationBlock
and sponsor
can be packed in single SLOT :SAVES 2000 GAS, 1 SLOT
uint48 is more than sufficient to hold block number.
File : main/packages/revolution/src/interfaces/ICultureIndex.sol 115: struct ArtPiece { uint256 pieceId; ArtPieceMetadata metadata; CreatorBps[] creators; address sponsor; bool isDropped; + uint48 creationBlock; - uint256 creationBlock; uint256 quorumVotes; uint256 totalERC20Supply; uint256 totalVotesSupply; 125: }
modifier
checks in constructors (saves 144000 GAS)SAVE: 144000 GAS on DEPLOYMENT
initializer
checkSince initializer modifier is used to make initialize function run only once in upgradeable contracts. But constructor runs only once already at the time of deployment so there is no need of initializer modifier in constructor.
SAVES ~24000 GAS
on DEPLOYMENTFile : main/packages/revolution/src/AuctionHouse.sol 95: constructor(address _manager) payable initializer {
SAVES 24000 GAS
on DEPLOYMENTFile : main/packages/revolution/src/CultureIndex.sol 92: constructor(address _manager) payable initializer {
SAVES 24000 GAS
on DEPLOYMENTFile : main/packages/revolution/src/ERC20TokenEmitter.sol 64: constructor( 65: address _manager, 66: address _protocolRewards, 67: address _protocolFeeRecipient 68: ) payable TokenEmitterRewards(_protocolRewards, _protocolFeeRecipient) initializer { 69: manager = IRevolutionBuilder(_manager); 70: }
SAVES 24000 GAS
on DEPLOYMENTFile : main/packages/revolution/src/MaxHeap.sol 30: constructor(address _manager) payable initializer { 31: manager = IRevolutionBuilder(_manager); 32: }
SAVES 24000 GAS
on DEPLOYMENTFile : main/packages/revolution/src/NontransferableERC20Votes.so 44: constructor(address _manager) payable initializer { 45: manager = IRevolutionBuilder(_manager); 46: }
SAVES 24000 GAS
on DEPLOYMENTFile : revolution/src/VerbsToken.sol 116: constructor(address _manager) payable initializer { 117: manager = IRevolutionBuilder(_manager); 118: }
modifier
checks in functions(saves 123500 GAS)nonReentrant
checkFunction not calling any external contract can't be in re-entrancy also function can only be called by onlyOwner/onlyMinter so no chance to re-enter here.
SAVES ~24700 GAS
when every time this function calledFile : main/packages/revolution/src/ERC20TokenEmitter.sol 309: function setCreatorsAddress(address _creatorsAddress) external override onlyOwner nonReentrant { 310: require(_creatorsAddress != address(0), "Invalid address"); 311: 312: emit CreatorsAddressUpdated(creatorsAddress = _creatorsAddress); 313: }
SAVES ~24700 GAS
when every time this function calledFile : main/packages/revolution/src/VerbsToken.sol 184: function burn(uint256 verbId) public override onlyMinter nonReentrant { 185: _burn(verbId); 186: emit VerbBurned(verbId); 187: }
SAVES ~24700 GAS
when every time this function calledFile : main/packages/revolution/src/VerbsToken.sol 209: function setMinter(address _minter) external override onlyOwner nonReentrant whenMinterNotLocked { 210: require(_minter != address(0), "Minter cannot be zero address"); 211: minter = _minter; 212: 213: emit MinterUpdated(_minter); 214: }
SAVES ~24700 GAS
when every time this function calledFile : main/packages/revolution/src/VerbsToken.sol 230: function setDescriptor( 231: IDescriptorMinimal _descriptor 232: ) external override onlyOwner nonReentrant whenDescriptorNotLocked { 233: descriptor = _descriptor; 234: 235: emit DescriptorUpdated(_descriptor); 236: }
SAVES ~24700 GAS
when every time this function calledFile : main/packages/revolution/src/VerbsToken.sol 252: function setCultureIndex(ICultureIndex _cultureIndex) external onlyOwner whenCultureIndexNotLocked nonReentrant { 253: cultureIndex = _cultureIndex; 254: 255: emit CultureIndexUpdated(_cultureIndex); 256: }
Vote
struct as value from votes
mapping in place of that use only uin256 weight
as valueSAVE: 1 SLOT per new voterAddress as key
Since voterAddress is already used as key in internal mapping of votes
so no need to add again that address by adding Vote struct as value in internal mapping. It can save 1 SLOT per new voterAddress as key.
File : main/packages/revolution/src/CultureIndex.sol 68: // The mapping of all votes for a piece 69: mapping(uint256 => mapping(address => Vote)) public votes;
File : main/packages/revolution/src/interfaces/ICultureIndex.sol 131: struct Vote { 132: address voterAddress; 133: uint256 weight; 134: }
SAVE: 200 GAS, 2 SLOAD
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.
_calculateVoteWeight
and (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000
to save 2 SLOAD(~200 GAS)File : main/packages/revolution/src/CultureIndex.sol 209: function createPiece( ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray ) public returns (uint256) { 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]; + uint256 _totalCalculatedVoteWeight = _calculateVoteWeight( + erc20VotingToken.totalSupply(), + erc721VotingToken.totalSupply() + ); + uint256 _quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; newPiece.pieceId = pieceId; -226: newPiece.totalVotesSupply = _calculateVoteWeight( -227: erc20VotingToken.totalSupply(), -228: erc721VotingToken.totalSupply() -229: ); +226: newPiece.totalVotesSupply = _totalCalculatedVoteWeight; newPiece.totalERC20Supply = erc20VotingToken.totalSupply(); newPiece.metadata = metadata; newPiece.sponsor = msg.sender; newPiece.creationBlock = block.number; - newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; + newPiece.quorumVotes = _quorumVotes; for (uint i; i < creatorArrayLength; i++) { newPiece.creators.push(creatorArray[i]); } -240: emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply); +240: emit PieceCreated(pieceId, msg.sender, metadata, _quorumVotes, _totalCalculatedVoteWeight); // 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; + return pieceId; 248: }
File : main/packages/revolution/src/AuctionHouse.sol 54: address public WETH; 95: constructor(address _manager) payable initializer { 96: manager = IRevolutionBuilder(_manager); 97: } 113: function initialize( ... 143: WETH = _weth;//@audit this can be fixed based on chain so can be immutable in implementation }
File : main/packages/revolution/src/AuctionHouse.sol - address public WETH; + address public immutable WETH; - constructor(address _manager) payable initializer { - manager = IRevolutionBuilder(_manager); - } + constructor(address _manager, address _weth) payable initializer { + manager = IRevolutionBuilder(_manager); + WETH = _weth; + } function initialize( address _erc721Token, address _erc20TokenEmitter, address _initialOwner, - address _weth, IRevolutionBuilder.AuctionParams calldata _auctionParams ) external initializer { require(msg.sender == address(manager), "Only manager can initialize"); require(_weth != address(0), "WETH cannot be zero address"); __Pausable_init(); __ReentrancyGuard_init(); __Ownable_init(_initialOwner); _pause(); require( _auctionParams.creatorRateBps >= _auctionParams.minCreatorRateBps, "Creator rate must be greater than or equal to the creator rate" ); verbs = IVerbsToken(_erc721Token); erc20TokenEmitter = IERC20TokenEmitter(_erc20TokenEmitter); timeBuffer = _auctionParams.timeBuffer; reservePrice = _auctionParams.reservePrice; minBidIncrementPercentage = _auctionParams.minBidIncrementPercentage; duration = _auctionParams.duration; creatorRateBps = _auctionParams.creatorRateBps; entropyRateBps = _auctionParams.entropyRateBps; minCreatorRateBps = _auctionParams.minCreatorRateBps; - WETH = _weth; }
File : main/packages/revolution/src/VerbsToken.sol 281: function _mintTo(address to) internal returns (uint256) { 282: ICultureIndex.ArtPiece memory artPiece = cultureIndex.getTopVotedPiece(); // Check-Effects-Interactions Pattern // Perform all checks require( artPiece.creators.length <= cultureIndex.MAX_NUM_CREATORS(), "Creator array must not be > MAX_NUM_CREATORS" ); // Use try/catch to handle potential failure -292: try cultureIndex.dropTopVotedPiece() returns (ICultureIndex.ArtPiece memory _artPiece) { +292: try cultureIndex.dropTopVotedPiece() returns (ICultureIndex.ArtPiece memory /*_artPiece*/) { -293: artPiece = _artPiece; uint256 verbId = _currentVerbId++;
storage
instead of memory for structs/arrays saves gas (saves 4200 GAS)SAVES: 4200 GAS
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.
eliminating the cost of copying the struct from storage to memory
:SAVES 4200 GAS
-172: IAuctionHouse.Auction memory _auction = auction; +172: IAuctionHouse.Auction storage _auction = auction; -337: IAuctionHouse.Auction memory _auction = auction; +337: IAuctionHouse.Auction storage _auction = auction;
File : main/packages/revolution/src/AuctionHouse.sol 171: function createBid(uint256 verbId, address bidder) external payable override nonReentrant { +175: require(bidder != address(0), "Bidder cannot be zero address"); 172: IAuctionHouse.Auction memory _auction = auction; 173: 174: //require bidder is valid address -175: require(bidder != address(0), "Bidder cannot be zero address");
voter
for address(0) and then check pieceId
to _currentPieceId
to save gas because _currentPieceId
is a state variable so it is cheaper to check voter
firstFile : main/packages/revolution/src/CultureIndex.sol 307: function _vote(uint256 pieceId, address voter) internal { +309: require(voter != address(0), "Invalid voter address"); 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");
computeTotalReward
function to save gasFile : main/packages/protocol-rewards/src/abstract/RewardSplits.sol 40: function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) { 41: if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT(); 42: 43: return 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; 52: }
This code first calculates the total of all the Base Points (BPS) and then multiplies paymentAmountWei by the total BPS and finally divides the result by 10,000. This reduces the number of division operations to one, potentially optimizing gas usage in the contract execution.
function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) { if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT(); - return - (paymentAmountWei * BUILDER_REWARD_BPS) / - 10_000 + - (paymentAmountWei * PURCHASE_REFERRAL_BPS) / - 10_000 + - (paymentAmountWei * DEPLOYER_REWARD_BPS) / - 10_000 + - (paymentAmountWei * REVOLUTION_REWARD_BPS) / - 10_000; + uint256 totalBps = BUILDER_REWARD_BPS + + PURCHASE_REFERRAL_BPS + + DEPLOYER_REWARD_BPS + + REVOLUTION_REWARD_BPS; + uint256 totalReward = (paymentAmountWei * totalBps) / 10_000; + return totalReward; }
_vote
function to save gas!
opcode because it will work same as with !
opcode and cache totalVoteWeights[pieceId]
first to save 1 SLOAD :SAVES 2 ! opcode and 1 SLOAD(~100 GAS)
File : main/packages/revolution/src/CultureIndex.sol 307: function _vote(uint256 pieceId, address voter) internal { require(pieceId < _currentPieceId, "Invalid piece ID"); require(voter != address(0), "Invalid voter address"); require(!pieces[pieceId].isDropped, "Piece has already been dropped"); -311: require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted"); +311: require((votes[pieceId][voter].voterAddress == address(0)), "Already voted"); uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock); require(weight > minVoteWeight, "Weight must be greater than minVoteWeight"); votes[pieceId][voter] = Vote(voter, weight); +319: uint256 totalWeight = totalVoteWeights[pieceId]; -317: totalVoteWeights[pieceId] += weight; +317: totalVoteWeights[pieceId] = totalWeight + weight; -319: uint256 totalWeight = totalVoteWeights[pieceId]; // TODO add security consideration here based on block created to prevent flash attacks on drops? maxHeap.updateValue(pieceId, totalWeight); emit VoteCast(pieceId, voter, weight, totalWeight); 324: }
creatorRateBps
is also checked for 10000
in setCreatorRateBps
functionFile : main/packages/revolution/src/AuctionHouse.sol 233: function setMinCreatorRateBps(uint256 _minCreatorRateBps) external onlyOwner { 234: require(_minCreatorRateBps <= creatorRateBps, "Min creator rate must be less than or equal to creator rate"); -235: require(_minCreatorRateBps <= 10_000, "Min creator rate must be less than or equal to 10_000");
File : main/packages/revolution/src/CultureIndex.sol -438: if (from == address(0)) revert ADDRESS_ZERO(); 439: 440: // Ensure signature is valid 441: if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE();
Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly. It also wast gas as it requires the redeployment of the contract.
File : main/packages/revolution/src/AuctionHouse.sol 95: constructor(address _manager) payable initializer { 96: manager = IRevolutionBuilder(_manager); 97: }
File : main/packages/revolution/src/AuctionHouse.sol 92: constructor(address _manager) payable initializer { 93: manager = IRevolutionBuilder(_manager); 94: }
File : main/packages/revolution/src/ERC20TokenEmitter.sol 64: constructor( 65: address _manager, 66: address _protocolRewards, 67: address _protocolFeeRecipient 68: ) payable TokenEmitterRewards(_protocolRewards, _protocolFeeRecipient) initializer { 69: manager = IRevolutionBuilder(_manager); 70: }
File : main/packages/revolution/src/MaxHeap.sol 30: constructor(address _manager) payable initializer { 31: manager = IRevolutionBuilder(_manager); 32: }
File : main/packages/revolution/src/NontransferableERC20Votes.so 44: constructor(address _manager) payable initializer { 45: manager = IRevolutionBuilder(_manager); 46: }
File : revolution/src/VerbsToken.sol 116: constructor(address _manager) payable initializer { 117: manager = IRevolutionBuilder(_manager); 118: }
#0 - c4-pre-sort
2023-12-24T01:53:45Z
raymondfam marked the issue as high quality report
#1 - c4-judge
2024-01-07T13:42:57Z
MarioPoneder marked the issue as grade-a
#2 - c4-sponsor
2024-01-09T15:39:28Z
rocketman-21 (sponsor) acknowledged