Platform: Code4rena
Start Date: 07/07/2022
Pot Size: $75,000 USDC
Total HM: 32
Participants: 141
Period: 7 days
Judge: HardlyDifficult
Total Solo HM: 4
Id: 144
League: ETH
Rank: 25/141
Findings: 3
Award: $585.20
š Selected for report: 1
š Solo Findings: 0
š Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, Amithuddar, Avci, BowTiedWardens, Kthere, Limbooo, MEP, Ruhum, StyxRave, TomJ, Treasure-Seeker, TrungOre, Tutturu, Waze, bardamu, c3phas, cccz, codexploder, cryptphi, hake, horsefacts, hyh, oyc_109, pashov, peritoflores, scaraven, simon135, slywaters, sseefried, tofunmi, xiaoming90
1.3977 USDC - $1.40
https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L325
The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:
payable callbackpayable callback spends more than 2300 gas (which is only enough to emit something)If a user falls into one of the above categories, they'll be unable to receive funds from the vault in a migration wrapper. Inaccessible funds means loss of funds, which is Medium severity.
Both leave():
File: src/modules/Migration.sol #1 159 uint256 ethAmount = userProposalEth[_proposalId][msg.sender]; 160 proposal.totalEth -= ethAmount; 161 userProposalEth[_proposalId][msg.sender] = 0; 162 163 // Withdraws fractions from contract back to caller 164 IFERC1155(token).safeTransferFrom( 165 address(this), 166 msg.sender, 167 id, 168 amount, 169 "" 170 ); 171 // Withdraws ether from contract back to caller 172 payable(msg.sender).transfer(ethAmount);
and withdrawContribution() use payable.transfer()
File: src/modules/Migration.sol #2 320 // Temporarily store user's eth for the transfer 321 uint256 userEth = userProposalEth[_proposalId][msg.sender]; 322 // Udpates ether balance of caller 323 userProposalEth[_proposalId][msg.sender] = 0; 324 // Withdraws ether from contract back to caller 325 payable(msg.sender).transfer(userEth);
While they both use msg.sender, the funds are tied to the address that deposited them (lines 159 and 321), and there is no mechanism to change the owner of the funds to an alternate address.
Code inspection
Use address.call{value:x}() instead
#0 - stevennevins
2022-07-19T21:43:55Z
Duplicate of #325
#1 - HardlyDifficult
2022-07-28T15:43:30Z
After an unsuccessful migration, a multisig user (or other contract) may find their funds unrecoverable. Since a contract is able to enter a migration successfully and there is no way to specify an alternative send to address or migrate their escrowed funds to another account -- assets can be lost; as the warden points out here. I agree with Medium risk for this.
š Selected for report: xiaoming90
Also found by: 0x1f8b, 0x29A, 0x52, 0xA5DF, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xsanson, 0xsolstars, 242, 8olidity, Amithuddar, Aymen0909, Bnke0x0, BowTiedWardens, David_, Deivitto, ElKu, Funen, Hawkeye, IllIllI, JC, Kaiziron, Keen_Sheen, Kthere, Kulk0, Kumpa, Lambda, MEP, ReyAdmirado, Rohan16, Ruhum, Sm4rty, TomJ, Tomio, Treasure-Seeker, TrungOre, Tutturu, Viksaa39, Waze, _Adam, __141345__, ak1, apostle0x01, asutorufos, async, ayeslick, aysha, bbrho, benbaessler, berndartmueller, c3phas, cccz, chatch, cloudjunky, codexploder, cryptphi, delfin454000, dipp, durianSausage, dy, exd0tpy, fatherOfBlocks, hake, hansfriese, horsefacts, hubble, joestakey, jonatascm, kebabsec, kenzo, kyteg, mektigboy, neumo, oyc_109, pashov, pedr02b2, peritoflores, rajatbeladiya, rbserver, robee, rokinot, s3cunda, sach1r0, sahar, sashik_eth, scaraven, shenwilly, simon135, sorrynotsorry, sseefried, svskaushik, unforgiven, z3s, zzzitron
363.763 USDC - $363.76
| Issue | Instances | |
|---|---|---|
| [Lā01] | Input array lengths may differ | 3 |
| [Lā02] | Code does not properly bubble up reverts where the size is less than 68 | 1 |
| [Lā03] | Return values of transfer()/transferFrom() not checked | 2 |
| [Lā04] | Unused/empty receive()/fallback() function | 3 |
| [Lā05] | Missing checks for address(0x0) when assigning values to address state variables | 10 |
| [Lā06] | Open TODOs | 1 |
Total: 20 instances over 6 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Inconsistent hash definitions | 1 |
| [Nā02] | Re-declaration of function already defined in parent | 2 |
| [Nā03] | Misleading comments | 1 |
| [Nā04] | Document specific commit used when forking code | 1 |
| [Nā05] | Duplicated code | 1 |
| [Nā06] | Consider addings checks for signature malleability | 1 |
| [Nā07] | Code does not follow the checks-effects-interactions pattern | 1 |
| [Nā08] | Unnecessary return value from function | 1 |
| [Nā09] | Invalid/extraneous/optional function definitions in interface | 10 |
| [Nā10] | Contract implements interface without extending the interface | 1 |
| [Nā11] | require()/revert() statements should have descriptive reason strings | 1 |
| [Nā12] | public functions not called by the contract should be declared external instead | 1 |
| [Nā13] | Non-assembly method available | 4 |
| [Nā14] | constants should be defined rather than using magic numbers | 21 |
| [Nā15] | Redundant cast | 4 |
| [Nā16] | Missing event and or timelock for critical parameter change | 2 |
| [Nā17] | Constant redefined elsewhere | 1 |
| [Nā18] | Typos | 8 |
| [Nā19] | File is missing NatSpec | 3 |
| [Nā20] | NatSpec is incomplete | 5 |
| [Nā21] | Event is missing indexed fields | 21 |
Total: 91 instances over 21 issues
If the caller makes a copy-paste error, the lengths may be mismatchd and an operation believed to have been completed may not in fact have been completed
There are 3 instances of this issue:
File: src/modules/protoforms/BaseVault.sol 61 address[] calldata _tokens, 62: uint256[] calldata _amounts 80 address[] calldata _tokens, 81: uint256[] calldata _ids 101 address[] calldata _tokens, 102 uint256[] calldata _ids, 103: uint256[] calldata _amounts,
The check should be for a length of 68, not zero. A developer can return values less than 68, causing the current code to try and fail to convert it to a revert-with-reason
There is 1 instance of this issue:
File: src/utils/Multicall.sol 23 if (result.length == 0) revert(); 24 // If there is return data and the call wasn't successful, the call reverted with a reason or a custom error. 25: _revertedWithReason(result);
transfer()/transferFrom() not checkedNot all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment
The cases where these are currently used can be retried, so I've marked this as low. However, if any other developer writes code using them, it's possible that it may not be retryable, leading to it being a Medium bug.
There are 2 instances of this issue:
File: src/modules/protoforms/BaseVault.sol 65: IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);
File: src/references/TransferReference.sol 22: IERC20(_token).transfer(_to, _value);
receive()/fallback() functionIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))
There are 3 instances of this issue:
File: src/modules/Buyout.sol 53: receive() external payable {}
File: src/modules/Migration.sol 63: receive() external payable {}
File: src/Vault.sol 32: receive() external payable {}
address(0x0) when assigning values to address state variablesThere are 10 instances of this issue:
File: src/modules/Buyout.sol 47: registry = _registry; 48: supply = _supply; 49: transfer = _transfer;
File: src/modules/Migration.sol 59: registry = _registry;
File: src/modules/Minter.sol 18: supply = _supply;
File: src/modules/protoforms/BaseVault.sol 25: registry = _registry;
File: src/references/SupplyReference.sol 16: registry = _registry;
File: src/targets/Supply.sol 17: registry = _registry;
File: src/utils/Metadata.sol 17: token = _token;
File: src/Vault.sol 95: owner = _newOwner;
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: src/utils/MerkleBase.sol 24: // TODO: This can be aesthetically simplified with a switch. Not sure it will
The ones in this file call keccak256(), whereas the ones in other files use hex literals with comments. The code should be consistent throughout
There is 1 instance of this issue:
File: src/constants/Permit.sol 10 bytes32 constant PERMIT_TYPEHASH = keccak256( 11 "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)" 12: );
There are 2 instances of this issue:
File: src/interfaces/IBaseVault.sol /// @audit same as defined in `IProtoform` 37 function deployVault( 38 uint256 _fractionSupply, 39 address[] memory _modules, 40 address[] calldata _plugins, 41 bytes4[] calldata _selectors, 42 bytes32[] memory _mintProof 43: ) external returns (address vault);
File: src/interfaces/IFERC1155.sol /// @audit same as defined in `IERC1155` 106 function safeTransferFrom( 107 address _from, 108 address _to, 109 uint256 _id, 110 uint256 _amount, 111 bytes memory _data 112: ) external;
The increment cannot overflow due to length being less than uint256 max, not due to human time scales
There is 1 instance of this issue:
File: src/utils/Multicall.sol 30 // cannot realistically overflow on human timescales 31 unchecked { 32 ++i; 33: }
I believe the version forked is this one. Including the full URL to it will help auditors to do a better job, and will make future updates easier
There is 1 instance of this issue:
File: src/utils/MerkleBase.sol 5: /// @author Modified from Murky (https://github.com/dmfxyz/murky/blob/main/src/common/MurkyBase.sol)
The code below is the same as what appears here. Common code should be refactored into a shared function
There is 1 instance of this issue:
File: src/Vault.sol 142 function _revertedWithReason(bytes memory _response) internal pure { 143 assembly { 144 let returndata_size := mload(_response) 145 revert(add(32, _response), returndata_size) 146 } 147: }
Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly
There is 1 instance of this issue:
File: src/FERC1155.sol 126 address signer = ecrecover(digest, _v, _r, _s); 127 128 if (signer == address(0) || signer != _owner) 129: revert InvalidSignature(signer, _owner);
Even through FERC721 doesn't have the opportunity for reentrancy here, the best coding practice is to follow the check-effects-interaction pattern
There is 1 instance of this issue:
File: src/FERC1155.sol 85 _mint(_to, _id, _amount, _data); 86: totalSupply[_id] += _amount;
The _execute() function reverts if there is a failure, so it's not clear why it returns a success variable
There is 1 instance of this issue:
File: src/Vault.sol 134 // Revert if execution was unsuccessful 135 if (!success) { 136 if (response.length == 0) revert ExecutionReverted(); 137 _revertedWithReason(response); 138: }
There are 10 instances of this issue:
File: src/interfaces/IERC1155.sol /// @audit uri(uint256) isn't defined with those arguments in the standard IERC1155 definition 59: function uri(uint256 _id) external view returns (string memory);
File: src/interfaces/IERC20.sol /// @audit DOMAIN_SEPARATOR() isn't defined with those arguments in the standard IERC20 definition 13: function DOMAIN_SEPARATOR() external view returns (bytes32); /// @audit decimals() isn't defined with those arguments in the standard IERC20 definition 21: function decimals() external view returns (uint8); /// @audit name() isn't defined with those arguments in the standard IERC20 definition 23: function name() external view returns (string memory); /// @audit nonces(address) isn't defined with those arguments in the standard IERC20 definition 25: function nonces(address) external view returns (uint256); /// @audit permit(address,address,uint256,uint256,uint8,bytes32,bytes32) isn't defined with those arguments in the standard IERC20 definition 27 function permit( 28 address _owner, 29 address _spender, 30 uint256 _value, 31 uint256 _deadline, 32 uint8 _v, 33 bytes32 _r, 34: bytes32 _s /// @audit symbol() isn't defined with those arguments in the standard IERC20 definition 37: function symbol() external view returns (string memory);
File: src/interfaces/IERC721.sol /// @audit name() isn't defined with those arguments in the standard IERC721 definition 30: function name() external view returns (string memory); /// @audit symbol() isn't defined with those arguments in the standard IERC721 definition 54: function symbol() external view returns (string memory); /// @audit tokenURI(uint256) isn't defined with those arguments in the standard IERC721 definition 56: function tokenURI(uint256 _id) external view returns (string memory);
Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact
There is 1 instance of this issue:
File: src/utils/Metadata.sol /// @audit IERC1155MetadataURI.uri() 9: contract Metadata {
require()/revert() statements should have descriptive reason stringsThere is 1 instance of this issue:
File: src/utils/Multicall.sol 23: if (result.length == 0) revert();
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 is 1 instance of this issue:
File: src/utils/Metadata.sol 36: function uri(uint256 _id) public view returns (string memory) {
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length
There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary
There are 4 instances of this issue:
File: src/targets/Transfer.sol 231: if iszero(extcodesize(_token)) { 351: if iszero(extcodesize(_token)) { 494: if iszero(extcodesize(token)) {
File: src/Vault.sol 122: codeSize := extcodesize(_target)
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 21 instances of this issue:
File: src/FERC1155.sol /// @audit 100 247: royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100; /// @audit 20 315: return _getArgAddress(20);
File: src/modules/Buyout.sol /// @audit 100 86: uint256 buyoutPrice = (msg.value * 100) / /// @audit 100 /// @audit 100 87: (100 - ((depositAmount * 100) / totalSupply)); /// @audit 1000 209: (tokenBalance * 1000) / /// @audit 500 211: 500 /// @audit 5 451: nodes = new bytes32[](5); /// @audit 5 472: permissions = new Permission[](5); /// @audit 3 492: permissions[3] = Permission( /// @audit 4 498: permissions[4] = Permission(
File: src/modules/Migration.sol /// @audit 100 199: 100,
File: src/modules/protoforms/BaseVault.sol /// @audit 6 128: hashes = new bytes32[](6);
File: src/utils/MerkleBase.sol /// @audit 0x0 27: mstore(0x0, _left) /// @audit 0x20 28: mstore(0x20, _right) /// @audit 0x0 31: mstore(0x0, _right) /// @audit 0x20 32: mstore(0x20, _left) /// @audit 0x0 /// @audit 0x40 34: data := keccak256(0x0, 0x40) /// @audit 0x1 135: if (length & 0x1 == 1) {
File: src/VaultFactory.sol /// @audit 0xff 47: abi.encodePacked(bytes1(0xff), address(this), salt, creationHash)
The type of the variable is the same as the type to which the variable is being cast
There are 4 instances of this issue:
File: src/modules/Migration.sol /// @audit address(buyout) 208: IFERC1155(token).setApprovalFor(address(buyout), id, true);
File: src/VaultFactory.sol /// @audit address(vault) 87: address(vault)
File: src/VaultRegistry.sol /// @audit address(fNFT) 56: vault = _deployVault(_merkleRoot, address(fNFT), _plugins, _selectors); /// @audit address(fNFT) 73: vault = _deployVault(_merkleRoot, address(fNFT), _plugins, _selectors);
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There are 2 instances of this issue:
File: src/FERC1155.sol 198 function setContractURI(string calldata _uri) external onlyController { 199 contractURI = _uri; 200: }
File: src/Vault.sol 86 function setMerkleRoot(bytes32 _rootHash) external { 87 if (owner != msg.sender) revert NotOwner(owner, msg.sender); 88 merkleRoot = _rootHash; 89: }
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There is 1 instance of this issue:
File: src/modules/Migration.sol /// @audit seen in src/modules/Buyout.sol 43: uint256 public constant PROPOSAL_PERIOD = 7 days;
There are 8 instances of this issue:
File: src/constants/Transfer.sol /// @audit ERC1155_safeTransferFrom_data_offset_ptr 25: * to some parent value. For example, ERC1155_safeTransferFrom_data_offset_ptr
File: src/interfaces/IMigration.sol /// @audit succesful 23: // Address for the new vault to migrate to (if buyout is succesful) /// @audit propoal 25: // Boolean status to check if the propoal is active /// @audit succesfully 29: // New fraction supply for a given vault that has succesfully migrated
File: src/interfaces/ITransfer.sol /// @audit falsey 6: /// @dev Emitted when an ERC-20 token transfer returns a falsey value
File: src/modules/Migration.sol /// @audit Udpates 322: // Udpates ether balance of caller
File: src/targets/Transfer.sol /// @audit BatchTransfer1155Params_ptr 557: // at location BatchTransfer1155Params_ptr
File: src/utils/SafeSend.sol /// @audit attemping 15: /// @param _to Address attemping to send to
There are 3 instances of this issue:
File: src/constants/Memory.sol
File: src/constants/Supply.sol
File: src/constants/Transfer.sol
There are 5 instances of this issue:
File: src/FERC1155.sol /// @audit Missing: '@return' 239 /// @param _id Token ID royalties are being updated for 240 /// @param _salePrice Sale price to calculate the royalty for 241 function royaltyInfo(uint256 _id, uint256 _salePrice) 242 external 243 view 244: returns (address receiver, uint256 royaltyAmount) /// @audit Missing: '@return' 289 /// @notice Getter for URI of a token type 290 /// @param _id ID of the token type 291 function uri(uint256 _id) 292 public 293 view 294 override(ERC1155, IFERC1155) 295: returns (string memory) /// @audit Missing: '@return' 322 /// @param _approved Approval status for the token type 323 /// @param _deadline Expiration of the signature 324 function _computePermitStructHash( 325 address _owner, 326 address _operator, 327 uint256 _id, 328 bool _approved, 329 uint256 _deadline 330: ) internal returns (bytes32) { /// @audit Missing: '@return' 348 /// @param _approved Approval status for the token type 349 /// @param _deadline Expiration of the signature 350 function _computePermitAllStructHash( 351 address _owner, 352 address _operator, 353 bool _approved, 354 uint256 _deadline 355: ) internal returns (bytes32) {
File: src/modules/protoforms/BaseVault.sol /// @audit Missing: '@return' 32 /// @param _selectors List of function selectors 33 /// @param _mintProof List of proofs to execute a mint function 34 function deployVault( 35 uint256 _fractionSupply, 36 address[] calldata _modules, 37 address[] calldata _plugins, 38 bytes4[] calldata _selectors, 39 bytes32[] calldata _mintProof 40: ) external returns (address vault) {
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question
There are 21 instances of this issue:
File: src/interfaces/IBaseVault.sol 12: event ActiveModules(address indexed _vault, address[] _modules);
File: src/interfaces/IBuyout.sol 55 event Start( 56 address indexed _vault, 57 address indexed _proposer, 58 uint256 _startTime, 59 uint256 _buyoutPrice, 60 uint256 _fractionPrice 61: ); 65: event SellFractions(address indexed _seller, uint256 _amount); 69: event BuyFractions(address indexed _buyer, uint256 _amount); 74: event End(address _vault, State _state, address indexed _proposer); 79: event Cash(address _vault, address indexed _casher, uint256 _amount); 83: event Redeem(address _vault, address indexed _redeemer);
File: src/interfaces/IERC1155.sol 6 event ApprovalForAll( 7 address indexed _owner, 8 address indexed _operator, 9 bool _approved 10: ); 25: event URI(string _value, uint256 indexed _id);
File: src/interfaces/IERC20.sol 6 event Approval( 7 address indexed _owner, 8 address indexed _spender, 9 uint256 _amount 10: ); 11: event Transfer(address indexed _from, address indexed _to, uint256 amount);
File: src/interfaces/IERC721.sol 11 event ApprovalForAll( 12 address indexed _owner, 13 address indexed _operator, 14 bool _approved 15: );
File: src/interfaces/IFERC1155.sol 21: event SetMetadata(address indexed _metadata, uint256 _id); 26 event SetRoyalty( 27 address indexed _receiver, 28 uint256 _id, 29 uint256 _percentage 30: ); 36 event SingleApproval( 37 address indexed _owner, 38 address indexed _operator, 39 uint256 _id, 40 bool _approved 41: );
File: src/interfaces/IMigration.sol 61 event FractionsMigrated( 62 address indexed _oldVault, 63 address indexed _newVault, 64 uint256 _proposalId, 65 uint256 _amount 66: ); 74 event VaultMigrated( 75 address indexed _oldVault, 76 address indexed _newVault, 77 uint256 _proposalId, 78 address[] _modules, 79 address[] _plugins, 80 bytes4[] _selectors 81: );
File: src/interfaces/IVaultRegistry.sol 33 event VaultDeployed( 34 address indexed _vault, 35 address indexed _token, 36 uint256 _id 37: );
File: src/interfaces/IVault.sol 25: event Execute(address indexed _target, bytes _data, bytes _response); 29: event InstallPlugin(bytes4[] _selectors, address[] _plugins); 39: event UninstallPlugin(bytes4[] _selectors);
#0 - HardlyDifficult
2022-07-26T18:35:19Z
š Selected for report: joestakey
Also found by: 0x1f8b, 0x29A, 0xA5DF, 0xKitsune, 0xNazgul, 0xNineDec, 0xalpharush, 0xkatana, 0xsanson, 0xsolstars, 8olidity, Avci, Bnke0x0, BowTiedWardens, Chom, Deivitto, ElKu, Fitraldys, Funen, IllIllI, JC, Kaiziron, Lambda, Limbooo, MEP, NoamYakov, PwnedNoMore, RedOneN, ReyAdmirado, Rohan16, Ruhum, Saintcode_, Sm4rty, TomJ, Tomio, TrungOre, Tutturu, Waze, _Adam, __141345__, ajtra, apostle0x01, asutorufos, benbaessler, brgltd, c3phas, codexploder, cryptphi, delfin454000, dharma09, djxploit, durianSausage, fatherOfBlocks, giovannidisiena, gogo, horsefacts, hrishibhat, hyh, ignacio, jocxyen, jonatascm, karanctf, kebabsec, kyteg, m_Rassska, mektigboy, oyc_109, pedr02b2, rbserver, robee, rokinot, sach1r0, sashik_eth, simon135, slywaters
220.0426 USDC - $220.04
| Issue | Instances | |
|---|---|---|
| [Gā01] | Check the first bit of the number rather than using the modulo operator, to save gas | 1 |
| [Gā02] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 2 |
| [Gā03] | State variables only set in the constructor should be declared immutable | 70 |
| [Gā04] | Structs can be packed into fewer storage slots | 1 |
| [Gā05] | Using calldata instead of memory for read-only arguments in external functions saves gas | 15 |
| [Gā06] | Using storage instead of memory for structs/arrays saves gas | 2 |
| [Gā07] | State variables should be cached in stack variables rather than re-reading them from storage | 27 |
| [Gā08] | Multiple accesses of a mapping/array should use a local variable cache | 1 |
| [Gā09] | internal functions only called once can be inlined to save gas | 3 |
| [Gā10] | <array>.length should not be looked up in every loop of a for-loop | 8 |
| [Gā11] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 2 |
| [Gā12] | require()/revert() strings longer than 32 bytes cost extra gas | 2 |
| [Gā13] | Optimize names to save gas | 28 |
| [Gā14] | Using bools for storage incurs overhead | 1 |
| [Gā15] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 3 |
| [Gā16] | Using private rather than public for constants, saves gas | 5 |
| [Gā17] | Division by two should use bit shifting | 3 |
| [Gā18] | Empty blocks should be removed or emit something | 3 |
| [Gā19] | Use custom errors rather than revert()/require() strings to save gas | 5 |
| [Gā20] | Functions guaranteed to revert when called by normal users can be marked payable | 6 |
Total: 188 instances over 20 issues
Do the same sort of bit check as is on line 135
There is 1 instance of this issue:
File: src/utils/MerkleBase.sol 91: if (_node % 2 == 1) {
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves 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.
There are 2 instances of this issue:
File: src/FERC1155.sol 23 mapping(address => mapping(address => mapping(uint256 => bool))) 24 public isApproved; 25 /// @notice Mapping of metadata contracts for token ID types => metadata address 26 mapping(uint256 => address) public metadata; 27 /// @notice Mapping to track account nonces for metadata txs owner => nonces 28 mapping(address => uint256) public nonces; 29 /// @notice Mapping to track total supply for token ID types => totalSupply 30 mapping(uint256 => uint256) public totalSupply; 31 /// @notice Mapping to track royalty receivers for token ID types => royaltyAddress 32 mapping(uint256 => address) private royaltyAddress; 33 /// @notice Mapping to track the royalty percent for token ID types => royaltyPercent 34: mapping(uint256 => uint256) private royaltyPercent;
File: src/modules/Migration.sol 45 mapping(address => mapping(uint256 => Proposal)) public migrationInfo; 46 /// @notice Mapping of a proposal ID to a user's ether contribution 47 mapping(uint256 => mapping(address => uint256)) private userProposalEth; 48 /// @notice Mapping of a proposal ID to a user's fractions contribution 49 mapping(uint256 => mapping(address => uint256)) 50: private userProposalFractions;
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
There are 70 instances of this issue:
File: src/modules/Buyout.sol /// @audit registry 47: registry = _registry; /// @audit registry 61: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 71: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit registry 114: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 151: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 186: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 210: IVaultRegistry(registry).totalSupply(_vault) > /// @audit registry 246: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 267: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit registry 280: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 288: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit registry 319: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 351: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 387: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 424: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit supply 48: supply = _supply; /// @audit supply 219: IVault(payable(_vault)).execute(supply, data, _burnProof); /// @audit supply 264: IVault(payable(_vault)).execute(supply, data, _burnProof); /// @audit supply 294: IVault(payable(_vault)).execute(supply, data, _burnProof); /// @audit supply 476: supply, /// @audit supply 477: ISupply(supply).burn.selector /// @audit transfer 49: transfer = _transfer; /// @audit transfer 334: IVault(payable(_vault)).execute(transfer, data, _erc20TransferProof); /// @audit transfer 366: IVault(payable(_vault)).execute(transfer, data, _erc721TransferProof); /// @audit transfer 403: IVault(payable(_vault)).execute(transfer, data, _erc1155TransferProof); /// @audit transfer 441: transfer, /// @audit transfer 482: transfer, /// @audit transfer 483: ITransfer(transfer).ERC20Transfer.selector /// @audit transfer 488: transfer, /// @audit transfer 489: ITransfer(transfer).ERC721TransferFrom.selector /// @audit transfer 494: transfer, /// @audit transfer 495: ITransfer(transfer).ERC1155TransferFrom.selector /// @audit transfer 500: transfer, /// @audit transfer 501: ITransfer(transfer).ERC1155BatchTransferFrom.selector
File: src/modules/Migration.sol /// @audit buyout 58: buyout = payable(_buyout); /// @audit buyout 84: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 116: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 148: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 189: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 208: IFERC1155(token).setApprovalFor(address(buyout), id, true); /// @audit buyout 210: IBuyout(buyout).start{value: proposal.totalEth}(_vault); /// @audit buyout 225: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 263: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 301: (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault); /// @audit buyout 343: IBuyout(buyout).withdrawERC20( /// @audit buyout 367: IBuyout(buyout).withdrawERC721( /// @audit buyout 393: IBuyout(buyout).withdrawERC1155( /// @audit buyout 420: IBuyout(buyout).batchWithdrawERC1155( /// @audit buyout 438: (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo( /// @audit buyout 447: (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo( /// @audit registry 59: registry = _registry; /// @audit registry 81: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 95: proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply( /// @audit registry 111: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 143: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 184: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 200: IVaultRegistry(registry).totalSupply(_vault), /// @audit registry 235: address newVault = IVaultRegistry(registry).create( /// @audit registry 296: (address token, uint256 id) = IVaultRegistry(registry).vaultToToken( /// @audit registry 435: (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault); /// @audit registry 467: (address token, uint256 newFractionId) = IVaultRegistry(registry) /// @audit registry 470: uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
File: src/modules/Minter.sol /// @audit supply 18: supply = _supply; /// @audit supply 40: supply, /// @audit supply 60: IVault(payable(_vault)).execute(supply, data, _mintProof);
File: src/modules/protoforms/BaseVault.sol /// @audit registry 25: registry = _registry; /// @audit registry 43: vault = IVaultRegistry(registry).create(
File: src/VaultFactory.sol /// @audit implementation 21: implementation = address(new Vault()); /// @audit implementation 39: (uint256 creationPtr, uint256 creationSize) = implementation /// @audit implementation 69: vault = implementation.clone(salt, data);
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
There is 1 instance of this issue:
File: src/interfaces/IMigration.sol /// @audit Variable ordering with 10 slots instead of the current 11: /// uint256(32):startTime, uint256(32):targetPrice, uint256(32):totalEth, uint256(32):totalFractions, address[](32):modules, address[](32):plugins, bytes4[](32):selectors, uint256(32):oldFractionSupply, uint256(32):newFractionSupply, address(20):newVault, bool(1):isCommited, bool(1):fractionsMigrated 8 struct Proposal { 9 // Start time of the migration proposal 10 uint256 startTime; 11 // Target buyout price for the migration 12 uint256 targetPrice; 13 // Total ether contributed to the migration 14 uint256 totalEth; 15 // Total fractions contributed to the migration 16 uint256 totalFractions; 17 // Module contract addresses proposed for the migration 18 address[] modules; 19 // Plugin contract addresses proposed for the migration 20 address[] plugins; 21 // Function selectors for the proposed plugins 22 bytes4[] selectors; 23 // Address for the new vault to migrate to (if buyout is succesful) 24 address newVault; 25 // Boolean status to check if the propoal is active 26 bool isCommited; 27 // Old fraction supply for a given vault 28 uint256 oldFractionSupply; 29 // New fraction supply for a given vault that has succesfully migrated 30 uint256 newFractionSupply; 31 // Boolean status to check that the fractions have already been migrated 32 bool fractionsMigrated; 33: }
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 15 instances of this issue:
File: src/FERC1155.sol /// @audit _uri 68: function emitSetURI(uint256 _id, string memory _uri) external { /// @audit _data 79 function mint( 80 address _to, 81 uint256 _id, 82 uint256 _amount, 83 bytes memory _data 84: ) external onlyRegistry {
File: src/utils/MerkleBase.sol /// @audit _proof 43 function verifyProof( 44 bytes32 _root, 45 bytes32[] memory _proof, 46 bytes32 _valueToProve 47: ) public pure returns (bool) {
File: src/utils/Metadata.sol /// @audit _uri 24: function setURI(uint256 _id, string memory _uri) external {
File: src/VaultRegistry.sol /// @audit _plugins /// @audit _selectors 51 function create( 52 bytes32 _merkleRoot, 53 address[] memory _plugins, 54 bytes4[] memory _selectors 55: ) external returns (address vault) { /// @audit _plugins /// @audit _selectors 67 function createFor( 68 bytes32 _merkleRoot, 69 address _owner, 70 address[] memory _plugins, 71 bytes4[] memory _selectors 72: ) external returns (address vault) { /// @audit _plugins /// @audit _selectors 83 function createCollection( 84 bytes32 _merkleRoot, 85 address[] memory _plugins, 86 bytes4[] memory _selectors 87: ) external returns (address vault, address token) { /// @audit _plugins /// @audit _selectors 102 function createInCollection( 103 bytes32 _merkleRoot, 104 address _token, 105 address[] memory _plugins, 106 bytes4[] memory _selectors 107: ) external returns (address vault) {
File: src/Vault.sol /// @audit _selectors /// @audit _plugins 73: function install(bytes4[] memory _selectors, address[] memory _plugins) /// @audit _selectors 101: function uninstall(bytes4[] memory _selectors) external {
storage instead of memory for structs/arrays saves gasWhen 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
There are 2 instances of this issue:
File: src/VaultRegistry.sol 40: VaultInfo memory info = vaultToToken[msg.sender]; 118: VaultInfo memory info = vaultToToken[msg.sender];
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace 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.
There are 27 instances of this issue:
File: src/FERC1155.sol /// @audit _controller on line 303 305: : controllerAddress = _controller;
File: src/modules/Buyout.sol /// @audit registry on line 61 71: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit registry on line 186 210: IVaultRegistry(registry).totalSupply(_vault) > /// @audit registry on line 246 267: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit registry on line 280 288: uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault); /// @audit supply on line 476 477: ISupply(supply).burn.selector /// @audit transfer on line 482 483: ITransfer(transfer).ERC20Transfer.selector /// @audit transfer on line 483 488: transfer, /// @audit transfer on line 488 489: ITransfer(transfer).ERC721TransferFrom.selector /// @audit transfer on line 489 494: transfer, /// @audit transfer on line 494 495: ITransfer(transfer).ERC1155TransferFrom.selector /// @audit transfer on line 495 500: transfer, /// @audit transfer on line 500 501: ITransfer(transfer).ERC1155BatchTransferFrom.selector
File: src/modules/Migration.sol /// @audit buyout on line 189 208: IFERC1155(token).setApprovalFor(address(buyout), id, true); /// @audit buyout on line 208 210: IBuyout(buyout).start{value: proposal.totalEth}(_vault); /// @audit buyout on line 438 447: (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo( /// @audit registry on line 81 95: proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply( /// @audit registry on line 184 200: IVaultRegistry(registry).totalSupply(_vault), /// @audit registry on line 435 467: (address token, uint256 newFractionId) = IVaultRegistry(registry) /// @audit registry on line 467 470: uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
File: src/Vault.sol /// @audit owner on line 76 76: if (owner != msg.sender) revert NotOwner(owner, msg.sender); /// @audit owner on line 87 87: if (owner != msg.sender) revert NotOwner(owner, msg.sender); /// @audit owner on line 94 94: if (owner != msg.sender) revert NotOwner(owner, msg.sender); /// @audit owner on line 102 102: if (owner != msg.sender) revert NotOwner(owner, msg.sender); /// @audit owner on line 126 /// @audit owner on line 132 132: if (owner_ != owner) revert OwnerChanged(owner_, owner); /// @audit nonce on line 25 25: if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
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
There is 1 instance of this issue:
File: src/modules/Buyout.sol /// @audit buyoutInfo[_vault] on line 297 297: (buyoutInfo[_vault].state, buyoutInfo[_vault].proposer) = (
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 3 instances of this issue:
File: src/FERC1155.sol 324 function _computePermitStructHash( 325 address _owner, 326 address _operator, 327 uint256 _id, 328 bool _approved, 329 uint256 _deadline 330: ) internal returns (bytes32) { 350 function _computePermitAllStructHash( 351 address _owner, 352 address _operator, 353 bool _approved, 354 uint256 _deadline 355: ) internal returns (bytes32) {
File: src/Vault.sol 142: function _revertedWithReason(bytes memory _response) internal pure {
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 8 instances of this issue:
File: src/modules/Buyout.sol 454: for (uint256 i; i < permissions.length; ) {
File: src/modules/protoforms/BaseVault.sol 64: for (uint256 i = 0; i < _tokens.length; ) { 83: for (uint256 i = 0; i < _tokens.length; ) { 107: for (uint256 i = 0; i < _tokens.length; ++i) { 130: for (uint256 i; i < _modules.length; ++i) { 132: for (uint256 j; j < leaves.length; ++j) {
File: src/utils/MerkleBase.sol 51: for (uint256 i = 0; i < _proof.length; ++i) { 110: for (uint256 i; i < result.length; ++i) {
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 2 instances of this issue:
File: src/Vault.sol 78: for (uint256 i = 0; i < length; i++) { 104: for (uint256 i = 0; i < length; i++) {
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 are 2 instances of this issue:
File: src/utils/MerkleBase.sol 62: require(_data.length > 1, "wont generate root for single leaf"); 78: require(_data.length > 1, "wont generate proof for single leaf");
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 28 instances of this issue:
File: src/FERC1155.sol /// @audit burn(), emitSetURI(), mint(), permit(), permitAll(), setApprovalFor(), setContractURI(), setMetadata(), setRoyalties(), transferController(), royaltyInfo(), controller(), INITIAL_CONTROLLER(), VAULT_REGISTRY() 13: contract FERC1155 is Clone, ERC1155, IFERC1155 {
File: src/interfaces/IBaseVault.sol /// @audit batchDepositERC20(), batchDepositERC721(), batchDepositERC1155(), deployVault(), registry() 8: interface IBaseVault is IProtoform {
File: src/interfaces/IBuyout.sol /// @audit PROPOSAL_PERIOD(), REJECTION_PERIOD(), batchWithdrawERC1155(), buyFractions(), buyoutInfo(), cash(), end(), getLeafNodes(), getPermissions(), redeem(), registry(), sellFractions(), start(), supply(), transfer(), withdrawERC20(), withdrawERC721(), withdrawERC1155() 31: interface IBuyout is IModule {
File: src/interfaces/IFERC1155.sol /// @audit INITIAL_CONTROLLER(), NAME(), VAULT_REGISTRY(), VERSION(), burn(), contractURI(), controller(), emitSetURI(), isApproved(), metadata(), mint(), permit(), permitAll(), royaltyInfo(), setApprovalFor(), setContractURI(), setMetadata(), setRoyalties(), totalSupply(), transferController() 5: interface IFERC1155 {
File: src/interfaces/IMigration.sol /// @audit PROPOSAL_PERIOD(), batchMigrateVaultERC1155(), buyout(), commit(), generateMerkleTree(), join(), leave(), migrateFractions(), migrateVaultERC20(), migrateVaultERC721(), migrationInfo(), nextId(), propose(), registry(), settleFractions(), settleVault(), withdrawContribution() 36: interface IMigration {
File: src/interfaces/IMinter.sol /// @audit supply() 8: interface IMinter is IModule {
File: src/interfaces/IModule.sol /// @audit getLeafNodes(), getPermissions() 7: interface IModule {
File: src/interfaces/IProtoform.sol /// @audit deployVault(), generateMerkleTree() 7: interface IProtoform {
File: src/interfaces/ISupply.sol /// @audit mint(), burn() 5: interface ISupply {
File: src/interfaces/ITransfer.sol /// @audit ERC20Transfer(), ERC721TransferFrom(), ERC1155TransferFrom(), ERC1155BatchTransferFrom() 5: interface ITransfer {
File: src/interfaces/IVaultFactory.sol /// @audit deploy(), deployFor(), getNextAddress(), getNextSeed(), implementation() 5: interface IVaultFactory {
File: src/interfaces/IVaultRegistry.sol /// @audit burn(), create(), createCollection(), createCollectionFor(), createFor(), createInCollection(), factory(), fNFT(), fNFTImplementation(), mint(), nextId(), totalSupply(), uri(), vaultToToken() 23: interface IVaultRegistry {
File: src/interfaces/IVault.sol /// @audit execute(), init(), install(), merkleRoot(), methods(), nonce(), setMerkleRoot(), uninstall() 5: interface IVault {
File: src/modules/Buyout.sol /// @audit start(), sellFractions(), buyFractions(), end(), cash(), redeem(), withdrawERC20(), withdrawERC721(), withdrawERC1155(), batchWithdrawERC1155(), getLeafNodes(), getPermissions() 27: contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
File: src/modules/Migration.sol /// @audit propose(), join(), leave(), commit(), settleVault(), settleFractions(), withdrawContribution(), migrateVaultERC20(), migrateVaultERC721(), migrateVaultERC1155(), batchMigrateVaultERC1155(), migrateFractions(), generateMerkleTree() 28: contract Migration is
File: src/modules/Minter.sol /// @audit getLeafNodes(), getPermissions() 12: contract Minter is IMinter {
File: src/modules/protoforms/BaseVault.sol /// @audit deployVault(), batchDepositERC20(), batchDepositERC721(), batchDepositERC1155(), generateMerkleTree() 17: contract BaseVault is IBaseVault, MerkleBase, Minter, Multicall {
File: src/references/SupplyReference.sol /// @audit mint(), burn() 10: contract SupplyReference is ISupply {
File: src/references/TransferReference.sol /// @audit ERC20Transfer(), ERC721TransferFrom(), ERC1155TransferFrom(), ERC1155BatchTransferFrom() 12: contract TransferReference is ITransfer {
File: src/targets/Supply.sol /// @audit mint(), burn() 11: contract Supply is ISupply {
File: src/targets/Transfer.sol /// @audit ERC20Transfer(), ERC721TransferFrom(), ERC1155TransferFrom(), ERC1155BatchTransferFrom() 13: contract Transfer is ITransfer {
File: src/utils/MerkleBase.sol /// @audit hashLeafPairs(), verifyProof(), getRoot(), getProof(), log2ceil_naive() 7: abstract contract MerkleBase {
File: src/utils/Metadata.sol /// @audit setURI() 9: contract Metadata {
File: src/utils/Multicall.sol /// @audit multicall() 7: abstract contract Multicall {
File: src/utils/SelfPermit.sol /// @audit selfPermit(), selfPermitAll() 9: abstract contract SelfPermit {
File: src/VaultFactory.sol /// @audit deploy(), getNextAddress(), getNextSeed(), deployFor() 11: contract VaultFactory is IVaultFactory {
File: src/VaultRegistry.sol /// @audit burn(), create(), createFor(), createCollection(), createInCollection(), mint(), totalSupply(), uri(), createCollectionFor() 13: contract VaultRegistry is IVaultRegistry {
File: src/Vault.sol /// @audit init(), execute(), install(), setMerkleRoot(), uninstall() 11: contract Vault is IVault, NFTReceiver {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There is 1 instance of this issue:
File: src/FERC1155.sol 23 mapping(address => mapping(address => mapping(uint256 => bool))) 24: public isApproved;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 3 instances of this issue:
File: src/utils/MerkleBase.sol 188: ceil++;
File: src/Vault.sol 78: for (uint256 i = 0; i < length; i++) { 104: for (uint256 i = 0; i < length; i++) {
private rather than public for constants, saves gasIf 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
There are 5 instances of this issue:
File: src/FERC1155.sol 15: string public constant NAME = "FERC1155"; 17: string public constant VERSION = "1";
File: src/modules/Buyout.sol 35: uint256 public constant PROPOSAL_PERIOD = 2 days; 37: uint256 public constant REJECTION_PERIOD = 4 days;
File: src/modules/Migration.sol 43: uint256 public constant PROPOSAL_PERIOD = 7 days;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There are 3 instances of this issue:
File: src/utils/MerkleBase.sol 100: _node = _node / 2; 136: result = new bytes32[](length / 2 + 1); 142: result = new bytes32[](length / 2);
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.
There are 3 instances of this issue:
File: src/modules/Buyout.sol 53: receive() external payable {}
File: src/modules/Migration.sol 63: receive() external payable {}
File: src/Vault.sol 32: receive() external payable {}
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 5 instances of this issue:
File: src/FERC1155.sol 263 require( 264 msg.sender == _from || 265 isApprovedForAll[_from][msg.sender] || 266 isApproved[_from][msg.sender][_id], 267 "NOT_AUTHORIZED" 268: ); 275 require( 276 _to.code.length == 0 277 ? _to != address(0) 278 : INFTReceiver(_to).onERC1155Received( 279 msg.sender, 280 _from, 281 _id, 282 _amount, 283 _data 284 ) == INFTReceiver.onERC1155Received.selector, 285 "UNSAFE_RECIPIENT" 286: ); 297: require(metadata[_id] != address(0), "NO METADATA");
File: src/utils/MerkleBase.sol 62: require(_data.length > 1, "wont generate root for single leaf"); 78: require(_data.length > 1, "wont generate proof for single leaf");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 6 instances of this issue:
File: src/FERC1155.sol 56 function burn( 57 address _from, 58 uint256 _id, 59 uint256 _amount 60: ) external onlyRegistry { 79 function mint( 80 address _to, 81 uint256 _id, 82 uint256 _amount, 83 bytes memory _data 84: ) external onlyRegistry { 198: function setContractURI(string calldata _uri) external onlyController { 205 function setMetadata(address _metadata, uint256 _id) 206 external 207: onlyController 217 function setRoyalties( 218 uint256 _id, 219 address _receiver, 220 uint256 _percentage 221: ) external onlyController { 229 function transferController(address _newController) 230 external 231: onlyController