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: 59/141
Findings: 2
Award: $114.45
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
76.9763 USDC - $76.98
Internal functions can be removedInternal functions cannot be called from outside the contract. If it is unused in the contract, it can be removed.
Recommended mitigation:
There are 2 instances of this issue:
File: src/modules/Minter.sol 50 function _mintFractions( 51 address _vault, 52 address _to, 53 uint256 _fractionSupply, 54 bytes32[] calldata _mintProof 55: ) internal {
File: src/utils/SafeSend.sol 30 function _sendEthOrWeth(address _to, uint256 _value) internal { 31 if (!_attemptETHTransfer(_to, _value)) { 32 WETH(WETH_ADDRESS).deposit{value: _value}(); 33 WETH(WETH_ADDRESS).transfer(_to, _value); 34 } 35: }
nonces[_owner]++ returns the current nonces[_owner] before incrementingThis makes it possble for strucHash to be unchanged between different calls of permit() or permitAll() functions for each token. Further exploitation is thwarted, however, as the result of _computeDomainSeparator() (which is incorporated as part of the final digest) is guaranteed to be unique between different calls of the permit and permitAll functions.
Recommended mitigation:
nonces[_owner]++ with ++nonces[_owner] - this will increment the nonce before returning. This has the added benefit of saving gas.There are 2 instances of this issue:
File: src/FERC1155.sol 331 return 332 keccak256( 333 abi.encode( 334 PERMIT_TYPEHASH, 335 _owner, 336 _operator, 337 _id, 338 _approved, 339 nonces[_owner]++, 340 _deadline 341 ) 342: );
File: src/FERC1155.sol 356 return 357 keccak256( 358 abi.encode( 359 PERMIT_ALL_TYPEHASH, 360 _owner, 361 _operator, 362 _approved, 363 nonces[_owner]++, 364 _deadline 365 ) 366: );
nonce is 0If an attacker can make nonce equal to 0, anyone can transfer ownership of the vault to themselves.
Recommended mitigation:
There is 1 instance of this issue:
File: src/Vault.sol 23 /// @dev Initializes nonce and proxy owner 24 function init() external { 25 if (nonce != 0) revert Initialized(owner, msg.sender, nonce); 26 nonce = 1; 27 owner = msg.sender; 28 emit TransferOwnership(address(0), msg.sender); 29: }
🌟 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
37.4743 USDC - $37.47
| Issue | Instances |
|---|---|
++i uses less gas compared to i++ | 3 |
uint8 incures more gas overhead compared to uint256 | 2 |
Use custom errors instead of revert()/require() to save gas | 3 |
Add require() earlier in functions | 1 |
| Cache array length outside of loop | 4 |
internal functions that are only called once can be inlined to save gas | 5 |
| Return values directly without an intermediate return variable | 6 |
Let the default value 0 be applied to variables initialized to 0 | 2 |
Functions guaranteed to revert when called by normal users can be marked payable | 8 |
public functions not called by the contract should be declared external | 6 |
++i uses less gas compared to i++This is especially relevant for the use of i++ in for loops. This saves 6 gas per loop.
There are 3 instances of this issue:
File: src/Vault.sol 78 for (uint256 i = 0; i < length; i++) { 79 methods[_selectors[i]] = _plugins[i]; 80: }
File: src/Vault.sol 104 for (uint256 i = 0; i < length; i++) { 105 methods[_selectors[i]] = address(0); 106: }
File: src/utils/MerkelBase.sol 186 while (x > 0) { 187 x >>= 1; 188 ceil++; 189: }
uint8 incures more gas overhead compared to uint256There are 2 instances of this issue:
File: src/FERC1155.sol 98 function permit( 99 address _owner, 100 address _operator, 101 uint256 _id, 102 bool _approved, 103 uint256 _deadline, 104 uint8 _v, 105 bytes32 _r, 106 bytes32 _s 107: ) external {
File: src/FERC1155.sol 145 function permitAll( 146 address _owner, 147 address _operator, 148 bool _approved, 149 uint256 _deadline, 150 uint8 _v, 151 bytes32 _r, 152 bytes32 _s 153: ) external {
revert()/require() to save gasCustom errors are available from solidity version 0.8.4. The instances below match or exceed that version.
There are 3 instances of this issue:
File: src/FERC1155.sol 263 require( 264 msg.sender == _from || 265 isApprovedForAll[_from][msg.sender] || 267 isApproved[_from][msg.sender][_id], 268 "NOT_AUTHORIZED" 269: );
File: src/FERC1155.sol 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: );
File: src/FERC1155.sol 297: require(metadata[_id] != address(0), "NO METADATA");
require() earlier in functionsThis saves gas on operations between the start of the function and where the require function is actually being called.
There is 1 instance of this issue:
File: src/FERC1155.sol 276 require( 277 _to.code.length == 0 278 ? _to != address(0) 279 : INFTReceiver(_to).onERC1155Received( 280 msg.sender, 281 _from, 282 _id, 283 _amount, 284 _data 285 ) == INFTReceiver.onERC1155Received.selector, 286 "UNSAFE_RECIPIENT" 287: );
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration. To do this, create a variables containing the array length before the loop.
There are 4 instances of this issue:
File: src/modules/Buyout.sol 276: for (uint256 i; i < permissions.length; ) {
File: src/utils/MerkleBase.sol 51: for (uint256 i = 0; i < _proof.length; ++i) {
File: src/utils/MerkleBase.sol 63: while (_data.length > 1) {
File: src/utils/MerkleBase.sol 110: for (uint256 i; i < result.length; ++i) {
internal functions that are only called once can be inlined to save gasDepending on the function contents, this will save 20~40 gas by omiting two JUMP operations and stack operations needed for the function call.
There are 5 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) {
File: src/FERC1155.sol 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 { 143 assembly { 144 let returndata_size := mload(_response) 145 revert(add(32, _response), returndata_size) 146 } 147: }
File: src/utils/Multicall.sol 39 function _revertedWithReason(bytes memory _response) internal pure { 40 assembly { 41 let returndata_size := mload(_response) 42 revert(add(32, _response), returndata_size) 43 } 44: }
File: src/utils/SafeSend.sol 18 function _attemptETHTransfer(address _to, uint256 _value) 19 internal 20 returns (bool success) 21: {
Initializing a return variable for a function, then assigning a value to it requires more gas compared to simply returning the value, as long as the variable is not being used elsewhere in the function.
There are 6 instances of this issue:
File: src/Vault.sol 67: (success, response) = _execute(_target, _data);
File: src/VaultFactory.sol 26 function deploy() external returns (address payable vault) { 27 vault = deployFor(msg.sender); 28: }
File: src/VaultFactory.sol 49: vault = address(uint160(uint256(data)));
File: src/VaultRegistry.sol 51 function create( 52 bytes32 _merkleRoot, 53 address[] memory _plugins, 54 bytes4[] memory _selectors 55 ) external returns (address vault) { 56 vault = _deployVault(_merkleRoot, address(fNFT), _plugins, _selectors); 57: }
File: src/VaultRegistry.sol 83 function createCollection( 84 bytes32 _merkleRoot, 85 address[] memory _plugins, 86 bytes4[] memory _selectors 87 ) external returns (address vault, address token) { 88 (vault, token) = createCollectionFor( 89 _merkleRoot, 90 msg.sender, 91 _plugins, 92 _selectors 93 ); 94: }
File: src/VaultRegistry.sol 102 function createInCollection( 103 bytes32 _merkleRoot, 104 address _token, 105 address[] memory _plugins, 106 bytes4[] memory _selectors 107 ) external returns (address vault) { 108 address controller = FERC1155(_token).controller(); 109 if (controller != msg.sender) 110 revert InvalidController(controller, msg.sender); 111 vault = _deployVault(_merkleRoot, _token, _plugins, _selectors); 112: }
0 be applied to variables initialized to 0Letting the default value of 0 be initialized to variables costs less gas compared to initializing it to a value of 0.
There are 2 instances of this issue:
File: src/Vault.sol 78: for (uint256 i = 0; i < length; i++) {
File: src/utils/MerkleBase.sol 51: for (uint256 i = 0; i < _proof.length; ++i) {
payableFor example, a function modifier such as onlyController is used. Adding the payable marker will lower gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. Saves about 21 gas per call to the function, as well as saving on deployment cost.
There are 8 instances of this issue:
File: src/FERC1155.sol 56 function burn( 57 address _from, 58 uint256 _id, 59 uint256 _amount 60: ) external onlyRegistry {
File: src/FERC1155.sol 79 function mint( 80 address _to, 81 uint256 _id, 82 uint256 _amount, 83 bytes memory _data 84: ) external onlyRegistry {
File: src/FERC1155.sol 198: function setContractURI(string calldata _uri) external onlyController {
File: src/FERC1155.sol 205 function setMetadata(address _metadata, uint256 _id) 206 external 207 onlyController 208: {
File: src/FERC1155.sol 217 function setRoyalties( 218 uint256 _id, 219 address _receiver, 220 uint256 _percentage 221: ) external onlyController {
File: src/FERC1155.sol 229 function transferController(address _newController) 230 external 231 onlyController 232: {
File: src/VaultRegistry.sol 39 function burn(address _from, uint256 _value) external { 40 VaultInfo memory info = vaultToToken[msg.sender]; 41 uint256 id = info.id; 42 if (id == 0) revert UnregisteredVault(msg.sender); 43 FERC1155(info.token).burn(_from, id, _value); 44: }
File: src/VaultRegistry.sol 117 function mint(address _to, uint256 _value) external { 118 VaultInfo memory info = vaultToToken[msg.sender]; 119 uint256 id = info.id; 120 if (id == 0) revert UnregisteredVault(msg.sender); 121 FERC1155(info.token).mint(_to, id, _value, ""); 122: }
public functions not called by the contract should be declared externalContracts are allowed to override their parents' functions and change the visibility from external to public if required.
There are 6 instances of this issue:
File: src/FERC1155.sol 256 function safeTransferFrom( 257 address _from, 258 address _to, 259 uint256 _id, 260 uint256 _amount, 261 bytes memory _data 262: ) public override(ERC1155, IFERC1155) {
File: src/FERC1155.sol 291 function uri(uint256 _id) 292 public 293 view 294 override(ERC1155, IFERC1155) 295 returns (string memory) 296: {
File: src/utils/MerkleBase.sol 43 function verifyProof( 44 bytes32 _root, 45 bytes32[] memory _proof, 46 bytes32 _valueToProve 47: ) public pure returns (bool) {
File: src/utils/MerkleBase.sol 61: function getRoot(bytes32[] memory _data) public pure returns (bytes32) {
File: src/utils/MerkleBase.sol 73 function getProof(bytes32[] memory _data, uint256 _node) 74 public 75 pure 76 returns (bytes32[] memory) 77: {
File: src/utils/Metadata.sol 36 function uri(uint256 _id) public view returns (string memory) { 37 return tokenMetadata[_id]; 38: }