Fractional v2 contest - apostle0x01's results

[L-01] Unused receive() function will lock Ether in contract

Impact

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

Vault.sol:32:    receive() external payable {}
modules/Buyout.sol:53:    receive() external payable {}
modules/Migration.sol:66:    receive() external payable {}
mocks/MockSender.sol:11:    receive() external payable {}
mocks/MockEthReceiver.sol:5:    receive() external payable {}

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L32 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L53 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L66 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/mocks/MockSender.sol#L11 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/mocks/MockEthReceiver.sol#L5

[G-01] No need to explicitly initialize variables with default values

Impact

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.

POC

./src/utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
./src/Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
./src/Vault.sol:104:        for (uint256 i = 0; i < 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/modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {

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

Mitigation

As an example: for (uint256 i = 0; i < length;) { should be replaced with for (uint256 i; i < length;) {

[G-02] Cache storage values in memory

Impact

Anytime you are reading from storage more than once, it is cheaper to cache variables in memory. An SLOAD cost 100 GAS while MLOAD and MSTORE only cost 3 GAS.

This is especially true in for loops when using the length of a storage array as the condition being checked after each loop. Caching the array length in memory can yield significant gas savings when the array length is high.

// Before
for(uint 1; i < approvals.length; i++); // approvals is a storage variable

// After
uint memory numApprovals = approvals.length;
for(uint 1; i < numApprovals; i++);

POC

./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) {
./src/modules/Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {
./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/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) {

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L51 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L110 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L454 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L64 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L83 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L107 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L130 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L132

[G-03] Expressions for constant values such as a call to keccak256(), should use immutable

Impact

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant See https://github.com/ethereum/solidity/issues/9232 issue for a detail description of the issue Similar bug: https://github.com/code-423n4/2021-11-unlock-findings/issues/238

POC

./src/constants/Permit.sol

5: bytes32 constant DOMAIN_TYPEHASH = keccak256(
    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
);
10: bytes32 constant PERMIT_TYPEHASH = keccak256(
    "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)"
);
15: bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(
    "PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)"
);

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

[G-04] ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integers. This is because the pre-increment operation is cheaper (about 5 GAS per iteration). This is implemented in other sol files but looks like its missing from Vault.sol .

POC

Vault.sol

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

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

[G-05] Shorten require messages to less than 32 characters to save gas

Impact

Strings that are more than 32 characters will require more than 1 storage slot, costing more gas. Consider reducing the message length to less than 32 characters or use error codes,

POC

./src/utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
./src/utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");

Here, MerkleBase.sol:62 uses 34 bytes and L78 uses 35 bytes.

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