Fractional v2 contest - gogo's results

2022-07-fractional

Gas Optimisations Report

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If 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 9 instances of this issue:

File: /src/Vault.sol

24:    function init() external {
25:         if (nonce != 0) revert Initialized(owner, msg.sender, nonce);


73:    function install(bytes4[] memory _selectors, address[] memory _plugins)
74:        external
75:    {
76:        if (owner != msg.sender) revert NotOwner(owner, msg.sender);


86:    function setMerkleRoot(bytes32 _rootHash) external {
87:        if (owner != msg.sender) revert NotOwner(owner, msg.sender);


93:    function transferOwnership(address _newOwner) external {
94:        if (owner != msg.sender) revert NotOwner(owner, msg.sender);


101:   function uninstall(bytes4[] memory _selectors) external {
102:       if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L24

File: /src/FERC1155.sol

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

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L198

PUBLIC FUNCTIONS NOT CALLED BY THE CONTRACT SHOULD BE DECLARED EXTERNAL INSTEAD

Contracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so.

There are 8 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) {


291:   function uri(uint256 _id)
292:       public
293:       view
294:       override(ERC1155, IFERC1155)
295:       returns (string memory)

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L256-L262

File: /src/utils/MerkleBase.sol

43:    function verifyProof(
44:        bytes32 _root,
45:        bytes32[] memory _proof,
46:        bytes32 _valueToProve
47:    ) public pure returns (bool) {


61:    function getRoot(bytes32[] memory _data) public pure returns (bytes32) {


73:    function getProof(bytes32[] memory _data, uint256 _node)
74:        public
75:        pure
76:        returns (bytes32[] memory)

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L43-L47

File: /src/utils/Metadata.sol

36:    function uri(uint256 _id) public view returns (string memory) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/Metadata.sol#L36

File: /src/utils/SelfPermit.sol

18:    function selfPermit(
19:        address _token,
20:        uint256 _id,
21:        bool _approved,
22:        uint256 _deadline,
23:        uint8 _v,
24:        bytes32 _r,
25:        bytes32 _s
26:    ) public {


46:    function selfPermitAll(
47:        address _token,
48:        bool _approved,
49:        uint256 _deadline,
50:        uint8 _v,
51:        bytes32 _r,
52:        bytes32 _s
53:    ) public {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/SelfPermit.sol#L18-L26

<ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use 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; ) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L454

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) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L64

File: /src/utils/MerkleBase.sol

51:    for (uint256 i = 0; i < _proof.length; ++i) {

110:   for (uint256 i; i < result.length; ++i) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L51

USING > 0 COSTS MORE GAS THAN != 0 WHEN USED ON A UINT

This change saves 6 gas per comparison

There is 1 instance of this issue:

File: /src/utils/MerkleBase.sol

186:    while (x > 0) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L186

RETURN DIRECTLY INSTEAD OF STORING A RETURN MEMORY VARIABLE

There are 5 instances of this issue:

File: /src/Vault.sol

53:    ) external payable returns (bool success, bytes memory response) {
           ...
67:        (success, response) = _execute(_target, _data);

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L53

File: /src/Vault.sol

55:    ) external returns (address vault) {
56:        vault = _deployVault(_merkleRoot, address(fNFT), _plugins, _selectors);

  
87:    ) external returns (address vault, address token) {
88:        (vault, token) = createCollectionFor(
  
  
107:   ) external returns (address vault) {
           ...
111:       vault = _deployVault(_merkleRoot, _token, _plugins, _selectors); 

https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultRegistry.sol#L55-L56

File: /src/Vault.sol

181:   returns (bool started)
           ...
212:       started = true;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L181

EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

It is expected that the value should be converted into a constant value at compile time. But actually the expression is re-calculated each time the constant is referenced.

There are 3 instances of this issue:

File: /src/constants/Permit.sol

5:     bytes32 constant DOMAIN_TYPEHASH = keccak256(
6:         "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
7:     );

  
10:    bytes32 constant PERMIT_TYPEHASH = keccak256(
11:        "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)"
12:    );

  
15:    bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(
16:        "PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)"
17:    );

https://github.com/code-423n4/2022-07-fractional/blob/main/src/constants/Permit.sol#L5

Replace x <= y with x < y + 1

In the EVM, there is no opcode for >= or <=. When using less than or equal, two operations are performed: < and =. Using strict comparison operators hence saves gas

There is 1 instance of this issue:

File: /src/modules/Buyout.sol

203:   if (block.timestamp <= endTime)

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L203

STATE VARIABLES ONLY SET IN THE CONSTRUCTOR SHOULD BE DECLARED IMMUTABLE

Avoids a Gsset (20000 gas)

There are 8 instances of this issue:

File: /src/VaultFactory.sol

15:    address public implementation;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultFactory.sol#L15

File: /src/modules/Buyout.sol

29:    address public registry;
                                     
31:    address public supply;
                                     
33:    address public transfer;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L29

File: /src/modules/Migration.sol

37:    address payable public buyout;
                                     
39:    address public registry;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L37

File: /src/modules/Minter.sol

14:    address public supply;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Minter.sol#L14

File: /src/modules/protoforms/BaseVault.sol

19:    address public registry;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L19

X += Y/X -= Y COSTS MORE GAS THAN X = X + Y/X = X - Y

There are 15 instances of this issue:

File: /src/FERC1155.sol

62:    totalSupply[_id] -= _amount;

86:    totalSupply[_id] += _amount;

270:   balanceOf[_from][_id] -= _amount;

271:   balanceOf[_to][_id] += _amount;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L62

File: /src/modules/Buyout.sol

139:   buyoutInfo[_vault].ethBalance -= ethAmount;

176:   buyoutInfo[_vault].ethBalance += msg.value;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L139

File: /src/modules/Migration.sol

123:   proposal.totalEth += msg.value;

124:   userProposalEth[_proposalId][msg.sender] += msg.value;

134:   proposal.totalFractions += _amount;

135:   userProposalFractions[_proposalId][msg.sender] += _amount;

156:   proposal.totalFractions -= amount;

160:   proposal.totalEth -= ethAmount;

497:   treeLength += IModule(_modules[i]).getLeafNodes().length;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L123

File: /src/utils/MerkleBase.sol

147:   for (uint256 i; i < length - 1; i += 2) {

190:   ceil -= pOf2; // see above

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L147

DEFAULT VALUE INITIALIZATION

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There are 6 instances of this issue:

File: /src/Vault.sol

78:    for (uint256 i = 0; i < length; i++) {

104:   for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L78

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) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L64

File: /src/utils/MerkleBase.sol

51:    for (uint256 i = 0; i < _proof.length; ++i) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L51

MULTIPLICATION/DIVISION BY TWO SHOULD USE BIT SHIFTING

x * 2 is equivalent to x << 1 and x / 2 is the same as x >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas

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);

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L100

REQUIRE()/REVERT() STRINGS LONGER THAN 32 BYTES COST EXTRA 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");

Use shorter error string message.

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L62

USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information

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");

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L263

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");

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L62

INTERNAL and PRIVATE FUNCTIONS ONLY CALLED ONCE CAN BE INLINED TO SAVE GAS

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 4 instances of this issue:

File: /src/Vault.sol
  
142:   function _revertedWithReason(bytes memory _response) internal pure {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L137

File: /src/utils/Multicall.sol
  
39:    function _revertedWithReason(bytes memory _response) internal pure {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/Multicall.sol#L25

File: /src/utils/Multicall.sol
  
324:   function _computePermitStructHash(
  
350:   function _computePermitAllStructHash(

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/Multicall.sol#L113 https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/Multicall.sol#L159

USING PRIVATE/INTERNAL RATHER THAN PUBLIC FOR CONSTANTS SAVES GAS

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

There are 6 instances of this issue:

File: /src/FERC1155.sol
  
15:    string public constant NAME = "FERC1155";
  
17:    string public constant VERSION = "1";

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#324

File: /src/modules/Buyout.sol
  
35:    uint256 public constant PROPOSAL_PERIOD = 2 days;
  
37:    uint256 public constant REJECTION_PERIOD = 4 days;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L35

File: /src/modules/Migration.sol
  
43:    uint256 public constant PROPOSAL_PERIOD = 7 days;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L43

File: /src/utils/SafeSend.sol
  
11:    address payable public constant WETH_ADDRESS =
12:        payable(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/SafeSend.sol#L11-L12

USE LITERAL ARRAYS INSTEAD OF CREATING AN ARRAY WITH SINGLE ITEM

There are 2 instances of this issue:

File: /src/modules/Minter.sol
  
- 24:    function getLeafNodes() external view returns (bytes32[] memory nodes) {
- 25:        nodes = new bytes32[](1);
- 26:        nodes[0] = keccak256(abi.encode(getPermissions()[0]));
- 27:    }
  
+ 24:    function getLeafNodes() external view returns (bytes32[] memory) {
+ 25:        return [keccak256(abi.encode(getPermissions()[0]))];
+ 26:    }

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Minter.sol#L25

File: /src/modules/Minter.sol

- 32:    function getPermissions()
- 33:        public
- 34:        view
- 35:        returns (Permission[] memory permissions)
- 36:    {
- 37:        permissions = new Permission[](1);
- 38:        permissions[0] = Permission(
- 39:            address(this),
- 40:            supply,
- 41:            ISupply.mint.selector
- 42:        );
- 43:    }
  
+ 32:    function getPermissions() public view returns (Permission[] memory) {
+ 33:        return [Permission(address(this), supply, ISupply.mint.selector)];
+ 34:    }

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Minter.sol#L37

PREFIX INCREMENTS ARE CHEAPER THAN POSTFIX INCREMENTS

++I costs less gas than I++, especially when it's used in for-loops (--I/I-- too). Saves 6 gas per instance

There are 3 instances of this issue:

File: /src/Vault.sol
  
78:    for (uint256 i = 0; i < length; i++) {
                                     
104:   for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L78

File: /src/utils/MerkleBase.sol
  
188:   ceil++;

https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L188

++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

The 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++) {

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L78