Fractional v2 contest - Rohan16's results

1.OPEN TODOS

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances

// Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L24

// actual codes

src/utils/MerkleBase.sol:24:            // TODO: This can be aesthetically simplified with a switch. Not sure it will

---

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

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.

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

Instances

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L107 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L130 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L132 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L51 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L110

//actual codes which shows the use.

src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) 
src/modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) 
src/modules/protoforms/BaseVault.sol:132:            for (uint256 j; j < leaves.length; ++j) 
src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) 
src/utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) 

---

2.IT COSTS MORE GAS TO INITIALIZE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0;

Instances

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L107 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/src/modules/protoforms/BaseVault.sol#L64 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L83 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/src/Vault.sol#L78 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L104 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L51

//actual codes which shows the use.
src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) 
src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) 
src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) 
src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) 
src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) 
src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i)

Recommendation:

I suggest removing explicit initializations for default values.

---

3. Preincrement Costs less gas(++i) as compared to postincrement(i++)

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper) i++ increments i and returns initial value of i. Which means uint i = 1; i++; // ==1 but i ==2 But ++i returns the actual incremented value: uint i = 1; ++i; // ==2 and i ==2 , no need for temporary variable here In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.

Instances:

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L188 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L339 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L363 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L133 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L508

//actual codes which shows the use.
src/utils/MerkleBase.sol:188:           ceil++
src/FERC1155.sol:339:                  nonces[_owner]++,
src/FERC1155.sol:363:                  nonces[_owner]++
src/modules/protoforms/BaseVault.sol:133:      hashes[counter++] = leaves[j]
src/modules/Migration.sol:508:                    hashes[counter++] = leaves[j]

Remediation:

Use Preincrement(++i) instead of Postincrement(i++) in code.

---

4. Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances Includes:

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L263-L268 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L275-286 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L62 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L78

//actual codes which shows the use 
src/FERC1155.sol:263-268
263:        require(
...            msg.sender == _from 
...            isApprovedForAll[_from][msg.sender] 
...               isApproved[_from][msg.sender][_id], 
267:"NOT_AUTHORIZED");

src/FERC1155.sol:275-286
  275:         require(
  ...              _to.code.length == 0
  ...
  ...               ) == INFTReceiver.onERC1155Received.selector,
  286:              "UNSAFE_RECIPIENT")
  
 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")

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

Instances

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L107 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/src/modules/protoforms/BaseVault.sol#L64 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L83 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/src/Vault.sol#L78 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L104 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L51

//actual codes which shows the use.
src/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) 
src/modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) 
src/modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) 
src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) 
src/Vault.sol:104:        for (uint256 i = 0; i < length; i++) 
src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i)

6. REDUCE THE SIZE OF ERROR MESSAGES (LONG REVERT STRINGS)

Instances

//Links to githubfile https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L62 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/utils/MerkleBase.sol#L78

//actual codes which shows the use 
 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")