Fractional v2 contest - joestakey's results

QA Report

Table of Contents

• summary

Low

• hash collision with abi.encodePacked
• Native transfer should be avoided
• Return value of ERC20.transferFrom unchecked
• Setters and constructors should check the input value
• Unused receive() functions

Non-critical

• Constants instead of magic numbers
• Events indexing
• Event should be emitted in setters
• Public functions can be external
• Redundant cast
• Signature malleability
• TODOs
• Visibility should be explicit

summary

Few vulnerabilities were found examining the contracts. The main concerns are with:

Low issues

hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

src/FERC1155.sol

394:             keccak256(
395:                 abi.encodePacked("\x19\x01", _domainSeparator, _structHash)
396:             );

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

Native transfer should be avoided

IMPACT

In Migration, the .transfer() method is used to transfer ETH.

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:

• The contract does not have a payable callback
• The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

src/modules/Migration.sol

172:         payable(msg.sender).transfer(ethAmount);
325:         payable(msg.sender).transfer(ethAmount);

TOOLS USED

Manual Analysis

MITIGATION

Use .call() to send ETH instead.

Return value of ERC20.transferFrom unchecked

IMPACT

Some ERC20 implementations do not revert upon a fail transfer/transferFrom call, but return false instead. Not checking the return values of these calls can hence lead to silent failures of tokens transfers.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

src/modules/protoforms/BaseVault.sol

65:             IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

TOOLS USED

Manual Analysis

MITIGATION

Check the return value of these calls to ensure they are not 0

Setters and constructors should check the input value

PROBLEM

Setters and constructors should check the input value for addresses - ie revert if address(0) is assigned to address variables.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

src/modules/protoforms/BaseVault.sol

24:     constructor(address _registry, address _supply) Minter(_supply) {
25:         registry = _registry;
26:     }

src/modules/Buyout.sol

42:     constructor(
43:         address _registry,
44:         address _supply,
45:         address _transfer
46:     ) {
47:         registry = _registry;
48:         supply = _supply;
49:         transfer = _transfer;
50:     }

src/modules/Migration.sol

58:         buyout = payable(_buyout);
59:         registry = _registry;

src/modules/Minter.sol

17:     constructor(address _supply) {
18:         supply = _supply;
19:     }

src/references/SupplyReference.sol

15:     constructor(address _registry) {
16:         registry = _registry;
17:     }

src/targets/Supply.sol

16:     constructor(address _registry) {
17:         registry = _registry;
18:     }

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks

Unused receive() functions

IMPACT

Vault and Buyout have an empty receive() function, but do not have any withdrawal function. Any ETH mistakenly sent to these contracts with empty msg.data would be locked.

SEVERITY

Low

PROOF OF CONCEPT

2 instances include:

src/Vault.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L32

32:     receive() external payable {}

src/modules/Buyout.sol#L53

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L53

53:     receive() external payable {}

TOOLS USED

Manual Analysis

MITIGATION

Removes these functions or implement the appropriate logic in these empty blocks

Non-critical issues

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values (100, 1000, etc) to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

7 instances include:

src/FERC1155.sol

247:         royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100;

src/modules/Buyout.sol

86:         uint256 buyoutPrice = (msg.value * 100) /
87:             (100 - ((depositAmount * 100) / totalSupply));

208:         if (
209:             (tokenBalance * 1000) /
210:                 IVaultRegistry(registry).totalSupply(_vault) >
211:             500
212:         )

src/modules/Migration.sol

198:         uint256 currentPrice = _calculateTotal(
199:             100,
200:             IVaultRegistry(registry).totalSupply(_vault),
201:             proposal.totalEth,
202:             proposal.totalFractions
203:         )

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use the maximum amount of indexed fields: up to three parameters. This makes it easier to filter for specific values in front-ends.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

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

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

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

src/interfaces/IVault.sol

25:     event Execute(address indexed _target, bytes _data, bytes _response);

33:     event TransferOwnership(
34:         address indexed _oldOwner,
35:         address indexed _newOwner
36:     );

src/interfaces/IVaultRegistry.sol

33:     event VaultDeployed(
34:         address indexed _vault,
35:         address indexed _token,
36:         uint256 _id
37:     );

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

src/FERC1155.sol

198:     function setContractURI(string calldata _uri) external onlyController

src/Vault.sol

86: function setMerkleRoot(bytes32 _rootHash) external 

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

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)

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Redundant cast

PROBLEM

In Migration.commit(), buyout is cast to type address, which is redundant as it is already of type address.

src/modules/Migration.sol

208:             IFERC1155(token).setApprovalFor(address(buyout), id, true);

SEVERITY

Non-Critical

TOOLS USED

Manual Analysis

MITIGATION

-208:             IFERC1155(token).setApprovalFor(address(buyout), id, true);
+208:             IFERC1155(token).setApprovalFor(buyout, id, true);

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

src/modules/Buyout.sol

208:         if (
209:             (tokenBalance * 1000) /

TOOLS USED

Manual Analysis

MITIGATION

Replace 1000 with 10e3

Signature malleability

PROBLEM

permit and permitAll in FERC1155 use Solidity's ecrecover to verify signatures. The EVM opcode associated with this function allows for malleable signatures and thus is susceptible to replay attacks. There is no direct threat to the protocol - these functions only approve operators - but it is still a good practice to avoid signature malleability.

SEVERITY

Non-Critical

PROOF OF CONCEPT

2 instances:

src/FERC1155.sol

126:             address signer = ecrecover(digest, _v, _r, _s);

171:             address signer = ecrecover(digest, _v, _r, _s);

TOOLS USED

Manual Analysis

MITIGATION

Use OpenZeppelin's ECDSA's library

TODOS

PROBLEM

There is an open TODO in MerkleBase.sol. It is merely a gas optimisation issue, but it should still be resolved before contract deployments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

src/utils/MerkleBase.sol

24:             // TODO: This can be aesthetically simplified with a switch. Not sure it will
25:             // save much gas but there are other optimizations to be had in here.

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODO comment

Visibility should be explicit

PROBLEM

Visibility of variables should be explicitly set.

SEVERITY

Non-Critical

PROOF OF CONCEPT

2 instances:

src/references/SupplyReference.sol

12:     address immutable registry;

src/targets/Supply.sol

13:     address immutable registry;

TOOLS USED

Manual Analysis

Gas Report

Table of Contents

• Array length should not be looked up in every iteration
• Bytes constant are cheaper than string constants
• Caching storage variables in local variables to save gas
• Caching mapping accesses in local variables to save gas
• Calldata instead of memory for RO function parameters
• Constant expressions
• Constants can be private
• Custom errors
• Empty blocks should emit an event
• Event fields are redundant
• Functions with access control cheaper if payable
• Immutable variables save storage
• Inline functions
• Mathematical optimizations
• Modifier instead of duplicate require
• Prefix increments
• Revert strings length
• Shifting cheaper than division
• Storage cheaper than memory
• Storage pointer for structs
• Transfers should be avoided if amount null
• unchecked arithmetic
• unnecessary computation

Array length should not be looked up in every iteration

IMPACT

It wastes gas to read an array's length in every iteration of a for loop, even if it is a memory or calldata array: 3 gas per read.

PROOF OF CONCEPT

8 instances:

src/modules/Buyout.sol

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

454:         for (uint256 i; i < permissions.length; )

src/modules/protoforms/BaseVault.sol

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

64:         for (uint256 i = 0; i < _tokens.length; )

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

83:         for (uint256 i = 0; i < _tokens.length; )

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

107:         for (uint256 i = 0; i < _tokens.length; ++i)

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

130:             for (uint256 i; i < _modules.length; ++i) 

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

132:             for (uint256 j; j < leaves.length; ++j) 

src/utils/MerkleBase.sol

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

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

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L110

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

TOOLS USED

Manual Analysis

MITIGATION

Caching the length in a variable before the for loop

Bytes constant are cheaper than string constants

IMPACT

If the string can fit into 32 bytes, then bytes32 is cheaper than string. string is a dynamically sized-type, which has current limitations in Solidity compared to a statically sized variable.

PROOF OF CONCEPT

2 instances:

src/FERC1155.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L15

15:     string public constant NAME = "FERC1155";

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L17

15:     string public constant VERSION = "1";

TOOLS USED

Manual Analysis

MITIGATION

Replace string constant with bytes(1..32) constant

Caching storage variables in local variables to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length can yield significant gas savings if the array length is high

PROOF OF CONCEPT

15 instances:

src/modules/Buyout.sol

scope: end()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L186

• registry is read twice:

186:         (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
210:                IVaultRegistry(registry)

scope: cash()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L246

• registry is read twice:

246:         (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
267:        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);

scope: redeem()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L280

• registry is read twice:

280:         (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
288:         uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);

scope: getPermissions()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L476

• supply is read twice:

476:             supply,
477:             ISupply(supply).burn.selector

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L482

• transfer is read 8 times:

482:             transfer,
483:             ITransfer(transfer).ERC20Transfer.selector
488:             transfer,
489:             ITransfer(transfer).ERC721TransferFrom.selector
494:             transfer,
495:             ITransfer(transfer).ERC1155TransferFrom.selector
500:             transfer,
501:             ITransfer(transfer).ERC1155BatchTransferFrom.selector

src/modules/Migrations.sol

scope: propose()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L81

• registry is read twice:

81:         (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
95:         proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(_vault);

scope: commit()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L184

• registry is read twice:

184:         (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
200:             IVaultRegistry(registry).totalSupply(_vault)

• buyout is read twice in the conditionnal if block:

208:             IFERC1155(token).setApprovalFor(address(buyout), id, true)
210:             IBuyout(buyout).start{value: proposal.totalEth}(_vault);

scope: settleVault()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L232

• proposal.modules is read twice:

232:         bytes32[] memory merkleTree = generateMerkleTree(proposal.modules);
247:             proposal.modules

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L237

• proposal.plugins is read twice:

237:             proposal.plugins
248:             proposal.plugins

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L238

• proposal.selectors is read twice:

238:             proposal.selectors
249:             proposal.selectors

scope: settleFractions()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L273

• proposal.newVault is read twice:

273:             proposal.newVault
283:             proposal.newVault

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L275

• proposal.newFractionSupply is read twice:

275:             proposal.newFractionSupply
285:             proposal.newFractionSupply

scope: migrateFractions()

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L435

• registry is read 3 times:

435:         (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault)
467:         (address token, uint256 newFractionId) = IVaultRegistry(registry)
470:         uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault)

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L438

• buyout is read twice:

438:         (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(_vault)
447:         (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(_vault);

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables using local variables.

Caching mapping accesses in local variables to save gas

IMPACT

Anytime you are reading from a mapping value more than once, it is cheaper in gas cost to cache it, by saving one gkeccak256 operation - 30 gas.

PROOF OF CONCEPT

1 instance:

src/FERC1155.sol

scope: uri()

• metadata[_id] is read twice:

297:        require(metadata[_id] != address(0), "NO METADATA");
298:         return IFERC1155(metadata[_id]).uri(_id)

TOOLS USED

Manual Analysis

MITIGATION

cache these mapping accesses using local variables.

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory,but it alleviates the compiler from the abi.decode() step that copies each index of the calldata to the memory index, each iteration costing 60 gas.

PROOF OF CONCEPT

20 instances:

src/FERC1155.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L68

68:     function emitSetURI(uint256 _id, string memory _uri) 

src/Vault.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L73

73:     function install(bytes4[] memory _selectors, address[] memory _plugins)

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L101

101:     function uninstall(bytes4[] memory _selectors)

src/VaultRegistry.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L53

53:         address[] memory _plugins
54:         bytes4[] memory _selectors

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L70

70:         address[] memory _plugins
71:         bytes4[] memory _selectors

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L85

85:         address[] memory _plugins
86:         bytes4[] memory _selectors

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L105

105:         address[] memory _plugins
106:         bytes4[] memory _selectors

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L150

150:         address[] memory _plugins
151:         bytes4[] memory _selectors

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L168

168:         address[] memory _plugins
169:         bytes4[] memory _selectors

src/modules/Migration.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L487

487:     function generateMerkleTree(address[] memory _modules)

src/utils/MerkleBase.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L44

44:     function verifyProof(bytes32[] memory _proof)

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L125

125:     function hashLevel(bytes32[] memory _data)

src/utils/Metadata.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/Metadata.sol#L24

24:     function setURI(uint256 _id, string memory _uri) 

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Constant expressions

IMPACT

Constant expressions are re-calculated each time they are in use, costing an extra 97 gas than a constant every time they are called.

PROOF OF CONCEPT

3 instances include:

src/constants/Permit.sol

5: bytes32 constant DOMAIN_TYPEHASH = keccak256(
6:     "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
7: );
8: 
9: /// @dev The EIP-712 typehash for the permit struct used by the contract
10: bytes32 constant PERMIT_TYPEHASH = keccak256(
11:     "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)"
12: );
13: 
14: /// @dev The EIP-712 typehash for the permit all struct used by the contract
15: bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(
16:     "PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)"
17: );

TOOLS USED

Manual Analysis

MITIGATION

Mark these as immutable instead of constant

Constants can be private

IMPACT

Marking constants as private save gas upon deployment, as the compiler does not have to create getter functions for these variables. It is worth noting that a private variable can still be read using either the verified contract source code or the bytecode. This may affect readability so this is left at the team's discretion

PROOF OF CONCEPT

6 instances:

src/VaultRegistry.sol

17:     address public immutable factory;
18:     /// @notice Address of FERC1155 token contract
19:     address public immutable fNFT;
20:     /// @notice Address of Implementation for FERC1155 token contract
21:     address public immutable fNFTImplementation;

src/modules/Buyout.sol

35:    uint256 public constant PROPOSAL_PERIOD = 2 days;
36:     /// @notice Time length of the rejection period
37:     uint256 public constant REJECTION_PERIOD = 4 days;

src/modules/Migration.sol

43:     uint256 public constant PROPOSAL_PERIOD = 7 days;

TOOLS USED

Manual Analysis

MITIGATION

Make the constants private instead of public

Custom Errors

IMPACT

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, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

5 instances:

src/FERC1155.sol

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

263:         require(
264:             msg.sender == _from ||
265:                 isApprovedForAll[_from][msg.sender] ||
266:                 isApproved[_from][msg.sender][_id],
267:             "NOT_AUTHORIZED"
268:         )

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L275-L286

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

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L297

297:         require(metadata[_id] != address(0), "NO METADATA");

src/utils/MerkleBase.sol

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

62:         require(_data.length > 1, "wont generate root for single leaf");

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L78

78:         require(_data.length > 1, "wont generate root for single leaf");

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in FERC1155.sol:

-297:         require(metadata[_id] != address(0), "NO METADATA");
+if (metadata[_id] == address(0)) {
+		revert NoMetadata();
+}

and define the custom error in the contract

+error NoMetadata();

• original gas costs:
  ╭───────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ FERC1155 contract ┆ ┆ ┆ ┆ ┆ │ ╞═══════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 1863562 ┆ 9340 ┆ ┆ ┆ ┆ │ ╰───────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

• new gas costs with the changes made above - ie one require statement changed into a custom error:
  ╭───────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ FERC1155 contract ┆ ┆ ┆ ┆ ┆ │ ╞═══════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 1857755 ┆ 9311 ┆ ┆ ┆ ┆ │ ╰───────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

• 5807 gas saved upon deployment.

Empty blocks should emit an event

PROBLEM

Empty blocks should emit an event, or revert. If not, they can simply be removed to save gas upon deployment. This is valid for receive() functions, but also constructors

PROOF OF CONCEPT

4 instances:

src/Vault.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L32

32:     receive() external payable {}

src/modules/Buyout.sol#L53

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L53

53:     receive() external payable {}

src/modules/Migration.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L63

63:     receive() external payable {}

src/utils/MerkleBase.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L8

8:     constructor() {}

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in these blocks, or remove them altogether.

Event fields are redundant

PROBLEM

block.timestamp and block.number are added to event information by default, explicitly adding them is a waste of gas.

PROOF OF CONCEPT

1 instance:

src/modules/Buyout.sol

100:        emit Start(
101:             _vault,
102:             msg.sender,
103:             block.timestamp,
104:             buyoutPrice,
105:             fractionPrice
106:         );

TOOLS USED

Manual Analysis

MITIGATION

Remove the event field emitting block.timestamp, as it is redundant.

Functions with access control cheaper if payable

PROBLEM

A function with access control marked as payable will be cheaper for legitimate callers: the compiler removes checks for msg.value, saving approximately 20 gas per function call.

PROOF OF CONCEPT

Instances:

src/FERC1155.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L56-L60

56:     function burn(
57:         address _from,
58:         uint256 _id,
59:         uint256 _amount
60:     ) external onlyRegistry

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L79-L84

79:     function mint(
80:         address _to,
81:         uint256 _id,
82:         uint256 _amount,
83:         bytes memory _data
84:     ) external onlyRegistry

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

198:     function setContractURI(string calldata _uri) external onlyController 

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L205-L207

205:     function setMetadata(address _metadata, uint256 _id)
206:         external
207:         onlyController

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L217-L221

217:     function setRoyalties(
218:         uint256 _id,
219:         address _receiver,
220:         uint256 _percentage
221:     ) external onlyController 

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L229-L231

229:     function transferController(address _newController)
230:         external
231:         onlyController

src/Vault.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L76

76:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L87

87:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L94

94:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L102

102:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

TOOLS USED

Manual Analysis

MITIGATION

Mark these functions as payable

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwards, marking it as immutable can save a storage slot - 20,000 gas. This also saves 97 gas on every read access of the variable.

PROOF OF CONCEPT

8 instances:

src/VaultFactory.sol

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

15:     address public implementation

src/modules/Buyout.sol

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

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

src/modules/Migration.sol

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

37:     address payable public buyout
39:     address public registry

src/modules/Minter.sol

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

14:     address public supply;

src/modules/protoforms/BaseVault.sol

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

19:     address public registry

TOOLS USED

Manual Analysis

MITIGATION

Mark these variables as immutable.

Inline functions

PROBLEM

When we define internal functions to perform computation:

• The contract’s code size gets bigger
• the function call consumes more gas than executing it as an inlined function (part of the code, without the function call)

When it does not affect readability, it is recommended to inline functions in order to save gas

PROOF OF CONCEPT

3 instances:

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) 

src/Vault.sol

142:     function _revertedWithReason(bytes memory _response) internal pure 

TOOLS USED

Manual Analysis

MITIGATION

Inline these functions where they are called:

• gas costs before inlining: ╭───────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ FERC1155 contract ┆ ┆ ┆ ┆ ┆ │ ╞═══════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 1863562 ┆ 9340 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ permit ┆ 898 ┆ 26460 ┆ 25659 ┆ 49995 ┆ 12 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ permitAll ┆ 853 ┆ 27880 ┆ 25585 ┆ 49588 ┆ 13 │ ╰───────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯ ╭────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ Vault contract ┆ ┆ ┆ ┆ ┆ │ ╞════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 816851 ┆ 4112 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ execute ┆ 3585 ┆ 40629 ┆ 61371 ┆ 66336 ┆ 182 │ ╰────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

• gas costs after inlining: ╭───────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ FERC1155 contract ┆ ┆ ┆ ┆ ┆ │ ╞═══════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 1833333 ┆ 9189 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ permit ┆ 898 ┆ 26348 ┆ 25519 ┆ 49855 ┆ 12 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ permitAll ┆ 853 ┆ 27768 ┆ 25447 ┆ 49450 ┆ 13 │ ╰───────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯ ╭────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮ │ Vault contract ┆ ┆ ┆ ┆ ┆ │ ╞════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 815051 ┆ 4103 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ execute ┆ 3585 ┆ 40628 ┆ 61371 ┆ 66336 ┆ 182 │ ╰────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

In FERC1155.sol:

• 30,229 gas is saved upon deployment
• 112 gas is saved per permit call on average
• 112 gas is saved per permitAll call on average

In Vault.sol:

• 1,800 gas is saved upon deployment

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

15 instances include:

src/FERC1155.sol

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

62:        totalSupply[_id] -= _amount;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L86

86:        totalSupply[_id] += _amount;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L270-L271

270:         balanceOf[_from][_id] -= _amount;
271:         balanceOf[_to][_id] += _amount;

src/modules/Buyout.sol

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

139:         buyoutInfo[_vault].ethBalance -= ethAmount

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L176

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

src/modules/Migration.sol

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

123:         proposal.totalEth += msg.value;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L124

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

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L134

134:         proposal.totalFractions += _amount;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L135

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

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L156

156:         proposal.totalFractions -= _amount;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L160

160:         proposal.totalEth -= ethAmount;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L497

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

src/utils/MerkleBase.sol

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

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

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L190

190:             ceil -= pOf2;

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Modifier instead of duplicate require

PROBLEM

When a require statement is used multiple times, it is cheaper in deployment costs to use a modifier instead.

PROOF OF CONCEPT

2 instances where a modifier can be used:

src/Vault.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L76

76:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L87

87:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L94

94:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L102

102:         if (owner != msg.sender) revert NotOwner(owner, msg.sender);

src/utils/MerkleBase.sol

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

62:         require(_data.length > 1, "wont generate root for single leaf");

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L78

78:         require(_data.length > 1, "wont generate root for single leaf");

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops.

PROOF OF CONCEPT

2 instances:

src/Vault.sol

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

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

TOOLS USED

Manual Analysis

MITIGATION

change i++ to ++i.

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes. It costs an extra 9,500 gas per string exceeding that 32-byte size upon deployment.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include instances:

src/utils/MerkleBase.sol

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

62:         require(_data.length > 1, "wont generate root for single leaf");

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L78

78:         require(_data.length > 1, "wont generate root for single leaf");

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

Shifting cheaper than division

IMPACT

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

PROOF OF CONCEPT

3 instances:

src/utils/MerkleBase.sol

100:                 _node = _node / 2

136:                 result = new bytes32[](length / 2 + 1);

142:                 result = new bytes32[](length / 2)

TOOLS USED

Manual Analysis

MITIGATION

Replace / 2 with >>1

Storage cheaper than memory

PROBLEM

Reference types cached in memory cost more gas than using storage, as new memory is allocated for these variables, copying data from storage to memory.

PROOF OF CONCEPT

Instances:

src/VaultRegistry.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L40

40:         VaultInfo memory info = vaultToToken[msg.sender];

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L118

118:         VaultInfo memory info = vaultToToken[msg.sender];

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L128

128:         VaultInfo memory info = vaultToToken[_vault];

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L136

136:         VaultInfo memory info = vaultToToken[_vault];

• original gas costs with these VaultInfo memory info ╭────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ VaultRegistry contract ┆ ┆ ┆ ┆ ┆ │ ╞════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 3898606 ┆ 19409 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ burn ┆ 2349 ┆ 4218 ┆ 4255 ┆ 4255 ┆ 52 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ mint ┆ 51597 ┆ 54389 ┆ 54845 ┆ 54845 ┆ 107 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ totalSupply ┆ 1878 ┆ 1878 ┆ 1878 ┆ 1878 ┆ 203 |
  ╰────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• new gas costs with these four instances as VaultInfo storage info ╭────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ VaultRegistry contract ┆ ┆ ┆ ┆ ┆ │ ╞════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 3881997 ┆ 19326 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ burn ┆ 2268 ┆ 4153 ┆ 4190 ┆ 4190 ┆ 52 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ mint ┆ 51516 ┆ 54308 ┆ 54764 ┆ 54764 ┆ 107 │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ totalSupply ┆ 1833 ┆ 1833 ┆ 1833 ┆ 1833 ┆ 203 |
  ╰────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• 16,609 gas is saved upon deployment

• 80 gas is saved per mint call on average

• 65 gas is saved per burn call on average

• 45 gas is saved per totalSupply call.

TOOLS USED

Manual Analysis

MITIGATION

Use storage instead of memory

Storage pointer for structs

PROBLEM

Using a storage pointer is cheaper than reading a struct field several times.

PROOF OF CONCEPT

Instances:

src/modules/Buyout.sol

297:         (buyoutInfo[_vault].state, buyoutInfo[_vault].proposer) = (
298:             State.SUCCESS,
299:             msg.sender
300:         );

TOOLS USED

Manual Analysis

MITIGATION

Use a storage pointer

+        Auction storage _vaultInfo = buyoutInfo[_vault];
+       (_vaultInfo.state, _vaultInfo.proposer) = (
-297:         (buyoutInfo[_vault].state, buyoutInfo[_vault].proposer) = (
298:             State.SUCCESS,
299:             msg.sender
300:         );

• original gas costs
  ╭──────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ Buyout contract ┆ ┆ ┆ ┆ ┆ │ ╞══════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 2779003 ┆ 13880 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ redeem ┆ 4184 ┆ 30536 ┆ 41198 ┆ 41198 ┆ 8 │ ╰──────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• new gas costs
  ╭──────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ Buyout contract ┆ ┆ ┆ ┆ ┆ │ ╞══════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 2775995 ┆ 13865 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ redeem ┆ 4184 ┆ 30532 ┆ 41193 ┆ 41193 ┆ 8 │ ╰──────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• 3,008 gas is saved upon deployment

• 5 gas is saved per redeem call on average

Transfers should be avoided if amount null

IMPACT

Gas can be saved by avoid ERC20.transfer function calls when the amount to be transferred is 0

PROOF OF CONCEPT

Instances include:

src/modules/Buyout.sol

129:         IERC1155(token).safeTransferFrom(
130:             msg.sender,
131:             address(this),
132:             id,
133:             _amount,
134:             ""
135:         );

There is no check that _amount is not zero ( it is a function argument)

141:         _sendEthOrWeth(msg.sender, ethAmount);

In the case _amount was zero, ethAmount would be zero too

TOOLS USED

Manual Analysis

MITIGATION

Add checks to ensure the _amount is not 0

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances:

src/Vault.sol

i is cannot overflow as it is a for loop

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

i is cannot overflow as it is a for loop

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

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

Redundant external calls waste gas.

PROOF OF CONCEPT

Instances:

src/modules/Migration.sol

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L438

• buyoutInfo is called twice:

438:         (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(_vault)
447:         (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(_vault);

TOOLS USED

Manual Analysis

MITIGATION

Replace

-438:         (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
-439:             _vault
-440:         );
+438:         (, address proposer, State current, , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
+439:             _vault
+440:         );
441:         State required = State.SUCCESS;
442:         if (current != required) revert IBuyout.InvalidState(required, current);
443:         // Reverts if proposer of buyout is not this contract
444:         if (proposer != address(this)) revert NotProposalBuyout();
445: 
446:         // Gets the last total supply of fractions for the vault
-447:         (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
-448:             _vault
-449:         );

• gas costs before amendment

╭──────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ Migration contract ┆ ┆ ┆ ┆ ┆ │ ╞══════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 3202385 ┆ 15886 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ migrateFractions ┆ 4079 ┆ 15056 ┆ 5786 ┆ 39226 ┆ 6 │ ╰──────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• gas costs after amendment

╭──────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮ │ Migration contract ┆ ┆ ┆ ┆ ┆ │ ╞══════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡ │ Deployment Cost ┆ Deployment Size ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ 3166145 ┆ 15705 ┆ ┆ ┆ ┆ │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │ ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤ │ migrateFractions ┆ 4094 ┆ 14384 ┆ 5786 ┆ 36970 ┆ 6 │ ╰──────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

• 36,240 gas is saved upon deployment
• 672 gas is saved per migrateFractions call on average