Fractional v2 contest - Deivitto's results

1. Missing indexed event parameters

a. Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

b. Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

InstallPlugin and UninstallPlugin events do not use indexed parameters.

c. Github Permalinks:

https://github.com/code-423n4/2022-07-fractional/blob/main/src/interfaces/IVault.sol#L29 https://github.com/code-423n4/2022-07-fractional/blob/main/src/interfaces/IVault.sol#L39

d. Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

2. Use of magic numbers is confusing and risky

a. Summary:

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

b. Details:

uint values in the operation are hardcoded and would be more readable and maintainable if declared as a constant

c. Github Permalinks:

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

d. Mitigation:

Replace magic hardcoded numbers with declared constants.

3. Missing events for critical functions

a. Summary:

Critical functions do not emit events.

b. Details:

Events should be emitted for critical functions that are access-controlled, setters of protocol parameters or those affecting protocol in significant ways such as transfers, mints or withdrawals:

c. Github Permalinks:

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

d. Mitigation:

Emit appropriate events with necessary parameters.

4. Naming and description of functions and variables

a. Summary:

Names and descriptions of functions and variables should be distinct and clearly indicate their purpose and effects to improve code readability, development and maintenance. They should also follow best-practices and be consistent when it comes to naming convention.

b. Details:

_controller and controller_variable names are very similar.

c. Github Permalinks:

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

d. Mitigation:

Use distinct/clear names and descriptions for functions and variables. Consider following best-practices and be consistent when it comes to naming convention to improve the readability, development, maintenance and not create any harmful confusion.

5. Single-step ownership change is risky

a. Summary:

Single-step ownership change in transferOwnership is risky

b. Details:

When privileged roles are being changed, it is recommended to follow a two-step approach:

1. The current privileged role proposes a new address for the change
2. The newly proposed address then claims the privileged role in a separate transaction. This two-step change allows accidental proposals to be corrected instead of leaving the system operationally with no/malicious privileged role.

For e.g., in a single-step change, if the current admin accidentally changes the new admin to a zero-address or an incorrect address (where the private keys are not available), the system is left without an operational admin and will have to be redeployed. transferOwnership implements a single-step ownership change.

c. Github Permalinks:

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

d. Mitigation:

Single-step ownership change can be overridden to a two-step change.

1. Public function visibility can be made external

a. Summary:

Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.

b. Details:

If a function is never called from the contract it should be marked as external. This will save gas.

c. Github Permalinks: https://github.com/code-423n4/2022-07-fractional/tree/main/src/utils/SelfPermit.sol#L46-L63

https://github.com/code-423n4/2022-07-fractional/tree/main/src/utils/SelfPermit.sol#L18-L37 https://github.com/code-423n4/2022-07-fractional/tree/main/src/utils/Metadata.sol#L39-L41 https://github.com/code-423n4/2022-07-fractional/tree/main/src/utils/MerkleBase.sol#L43-L56

d. Mitigation: Consider changing visibility from public to external.

2. Gas optimization by not initialising loop indices

a. Summary:

Loop indices need not be initialised to 0.

b. Details:

Loop indices are 0 valued by default and there is no need to explicitly initialise them again to 0. This saves gas.

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

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L83 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L107 https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/MerkleBase.sol#L51 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L78

d. Mitigation: Remove explicit initialization to 0 for loop indices.

3. Iterator in for loops may be optimized

a. Summary:

The iterator on the for loops can be changed to ++i, this would affect only to gas used as it the last operation in the loop. Also the use of unchecked decreases gas cost

b. Details:

b1) i++ should be change to ++i for gas optimization because it must increment a value and "return" the old value, so it may require holding two numbers in memory. ++i only ever uses one number in memory. b2) following with the code style, change i++ to ++i then put in a unchecked block at the very end of the for loop block, so it doesn't check for the overflow value for an extra gas optimization as in a for loop is not supposed to be able to reach the overflow value.

c. Github Permalinks:

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

d. Mitigation:

Use ++i in for loops in the iteration and unchecked for the overflow ignore