Fractional v2 contest - Waze's results

Lines of code

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

Vulnerability details

Impact

Use call instead of transfer to send ether. And return value must be checked if sending ether is successful or not. Sending ether with the transfer is no longer recommended.

Proof of Concept

function leave(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not inactive
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);


    // Gets the migration proposal for the given ID
    Proposal storage proposal = migrationInfo[_vault][_proposalId];
    // Updates fraction balances of the proposal and caller
    uint256 amount = userProposalFractions[_proposalId][msg.sender];
    proposal.totalFractions -= amount;
    userProposalFractions[_proposalId][msg.sender] = 0;
    // Updates ether balances of the proposal and caller
    uint256 ethAmount = userProposalEth[_proposalId][msg.sender];
    proposal.totalEth -= ethAmount;
    userProposalEth[_proposalId][msg.sender] = 0;


    // Withdraws fractions from contract back to caller
    IFERC1155(token).safeTransferFrom(
        address(this),
        msg.sender,
        id,
        amount,
        ""
    );
    // Withdraws ether from contract back to caller
    payable(msg.sender).transfer(ethAmount);
}

Tools Used

Manual review

Recommended Mitigation Steps

i suggest to change payable(msg.sender).transfer(ethAmount); to

 (bool result, ) = payable(msg.sendert).call{value: ethAmount}(""); 
  require(result, “Failed to send Ether”);

fix this to : https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L325

#1 missing comment receiver n royaltyamount impact return has a natspec comment which is missing the receiver and royaltyAmount function parameter. Issues with comments are low risk based on Code4rena risk categories.

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L238

Tool Used manual review

Recommendation Mitigation Steps add natspec comment on return in the function parameter receiver and royaltyAmount.

#2 missing return hash impact return has a natspec comment hash which is missing. it decrease readibility

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L345

Tool Used Manual review

Recommendation Mitigation Steps add natspec return of hash

#3 mint() doesnt have require of amount impact function incomplete due to the amount was missing

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L79

Tool Used manual review

Recommendation Mitigation Steps Add to mint() function: require(amount > 0, "Amount cannot be 0")

#4 permitAll missing requirement operator cannot be caller impact operator can be caller

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L145

Tool Used Manual review

Recommendation Mitigation Steps add to permitAll() function: require(operator != msg.sender, "operator can't be caller")

#5 tx.origin impact tx.origin is a global variable in Solidity which returns the address of the account that sent the transaction. Using the variable for authorization could make a contract vulnerable if an authorized account calls into a malicious contract. A call could be made to the vulnerable contract that passes the authorization check since tx.origin returns the original sender of the transaction which in this case is the authorized account.

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultFactory.sol#L62-L90

Tool Used

Recommendation Mitigation Steps tx.origin should not be used for authorization. Use msg.sender instead.

#6 missing indexed vault impact event is missing indexed fields.

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/interfaces/IBuyout.sol#L74-L83

Tool Used Manual review

Recommendation Mitigation Steps Add indexed at vault.

#7 missing immutable impact state registry, supply, and transfer can't be initialized

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Buyout.sol#L28-L33

Tool Used Manual review

Recommendation Mitigation Steps add immutable on the state

#8 missing immutable impact state buyout and registry can't be initialized

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L36-L39

Tool Used manual review

Recommendation Mitigation Steps add immutable on the state

#9 supply contract was missing and missing immutable impact state supply was missing makes state cant be initialized and missing immutable on state

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L37-L60

Tool Used Manual review Recommendation Mitigation Steps add address public immutable supply and add immutable to others

#10 missing immutable supply impact supply state can't be initialized

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

Tool Used manual review

Recommendation Mitigation Steps add immutable to the state

#11 missing immutable registry impact registry can't be initialized

proof of concept https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L24 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L345

Tool Used manual review

Recommendation Mitigation Steps add immutable to the registry state

#1 Visibility

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

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

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

change visibility from public to private or internal can save gas. so i recommend to change it.

#2 use calldata instead memory

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/interfaces/IBuyout.sol#L93-L95

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

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

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

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

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

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

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

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultRegistry.sol#L67-L71

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

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

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

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

In the external functions where the function argument is read-only, the function() has an inputed parameter that using memory, if this function didnt change the parameter, its cheaper to use calldata then memory. so we suggest to change it.

#3 cache controllerAddress

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L304-L305

cache controllerAddress to the memory becasue mload more cheaper than sload.

#4 increment

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/utils/MerkleBase.sol#L188

pre-increment more cheap than post increment. so use pre increment e.g. ++i

#5 looping

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

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

default uint is 0 so remove unnecassary explicit can reduce gas. pre increment e.g ++i more cheaper gas than post increment e.g i++. i suggest to use pre increment.

#6 use storage instead memory

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

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

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

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

Use storage instead of memory to reduce the gas fee. i suggest to change from

    VaultInfo memory info = vaultToToken[msg.sender];

to

    VaultInfo storage info = vaultToToken[msg.sender];

#7 cache IVault(vault)

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultRegistry.sol#L173-L174

cache to the memory because IVault(vault) use multiple times.it can reduce the gas.

    IVault(vault).setMerkleRoot(_merkleRoot);
    IVault(vault).install(_selectors, _plugins);

#8 sort the struct

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/interfaces/IBuyout.sol#L15

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/interfaces/IMigration.sol#L8

sort the struct can reduce the gas . i suggest to change from

struct Auction {
// Time of when buyout begins
uint256 startTime;
// Address of proposer creating buyout
address proposer;
// Enum state of the buyout auction
State state;
// Price of fractional tokens
uint256 fractionPrice;
// Balance of ether in buyout pool
uint256 ethBalance;
// Total supply recorded before a buyout started
uint256 lastTotalSupply;
}

to

struct Auction {
// Time of when buyout begins
uint256 startTime;
// Enum state of the buyout auction
State state;
// Price of fractional tokens
uint256 fractionPrice;
// Balance of ether in buyout pool
uint256 ethBalance;
// Total supply recorded before a buyout started
uint256 lastTotalSupply;
// Address of proposer creating buyout
address proposer;
 }

#9 cache tokens.length

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/modules/protoforms/BaseVault.sol#L107

cache the _tokens.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity. #10 cache modules.length

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

cache the modules.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity.

#11 cache leaves.length

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

cache the leaves.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity.

#12 cache data.length

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

cache the data.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity.