Fractional v2 contest - codexploder's results

Lines of code

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

Vulnerability details

Impact

User can deposit funds by joining a non existent proposal id. Once the proposal with this id is created, user would have joined a proposal which they might not have wanted

Proof of Concept

1. User accidentally provides proposal id 2 which is non existent, for Vault A while calling join function
2. No error is thrown and User funds are deposited for this non existent proposal
3. This becomes a problem once another user creates proposal id 2 for Vault A and commit it
4. Now User from Step 1 has deposited to a proposal in which he never wanted (proposal 1 and 2 on same vault may differ on price)

Recommended Mitigation Steps

Add below check to ensure that proposal actually exists

require(proposal.startTime>0, "No proposal found");

Lines of code

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

Vulnerability details

Impact

migrateVaultERC20/migrateVaultERC721 can lead to loss of all assets held by Vault

Proof of Concept

1. User starts a migration proposal
2. After PROPOSAL_PERIOD, proposal.targetPrice is achieved. Hence buyout process is started
3. Auction ends and buyout was success
4. Before User could call settleVault to set up the new vault, Attacker calls migrateVaultERC20/migrateVaultERC721/.. which migrates the tokens/nft to migrationInfo[_vault][_proposalId].newVault which is currently address(0)
5. Hence all tokens/assets are transferred to address 0 and are lost forever

Recommended Mitigation Steps

Add below check which make sure that Vault has been settled before migration

require(migrationInfo[_vault][_proposalId].newVault!=address(0),"Vault not settled");

Lines of code

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

Vulnerability details

Impact

User funds can stuck in case where multiple proposal exist on same Vault

Proof of Concept

1. User starts proposal id 1 vault A
2. User B joins this proposal id 1 and invests in it
3. User starts proposal id 2 vault A
4. User C joins this proposal id 2 and invests in it
5. Proposal id 2 is committed and buyout is success and migration gets success
6. Since migration is success User B cannot use withdraw and his funds get stuck

Recommended Mitigation Steps

In withdraw function, allow user to withdraw fund if proposal was not committed. In short, proposal was not committed or proposal was inactive - then allow withdraw

Lines of code

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

Vulnerability details

Impact

User should not be allowed to join/leave a proposal if block.timestamp > proposal.startTime + PROPOSAL_PERIOD

Proof of Concept

1. Observe the join function
2. Notice that user can join as long as commit is not being called
3. This means if commit is not called early then user can even join post proposal.startTime + PROPOSAL_PERIOD which is incorrect

Recommended Mitigation Steps

Add below check to revert if proposal end time has reached

require(block.timestamp <= proposal.startTime + PROPOSAL_PERIOD, "Time has ended");

Lines of code

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

Vulnerability details

Impact

The Merkle proof check can be bypassed by using fallback function which allows anyone to call _execute function (missing ACL checks). This allows attacker to execute plugins without caller being a module with permission to call or the owner.

Proof of Concept

1. User A makes an interaction (non existing method) such that fallback function on Vault.sol#L38 is invoked
2. User A has passed msg.sig as a valid plugin
3. _execute function is called which misses any ACL checks over the user passed plugin

Recommended Mitigation Steps

Instead of calling_execute, call to execute should be made including proof sent by user

Lines of code

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

Vulnerability details

Impact

The transfer function is bounded by 2300 gas limit. But some contract might need more than this gas which can lead to transfer failing

Proof of Concept

1. Observe that leave function makes use of transfer to send eth back to caller instead of using call

Recommended Mitigation Steps

Use call instead of transfer

 (bool success, ) = msg.sender.call{ethAmount}("");
    require(success, "Transfer failed.");

Plugin and Selector length are not matched

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

Issue: In _deployVault function, it is not verified that length for _plugins and _selectors is same which means the loop will run incorrect number of times

Recommendation: Add below check

require(_plugins.length==_selectors.length, "Incorrect length");

Missing zero address check

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

Issue: In transferOwnership function, add a zero address check otherwise this vault becomes useless as no plugins can now be installed

Recommendation: Add below check:

require(_newOwner!=address(0), "zero address");

Amount not given back to user post burn

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

Issue: redeem function is burning the totalSupply from vault but is not transferring the amount back to caller

Reverting early can save gas

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

Recommendation: royaltyInfo function: Revert if royaltyAddress[_id] is address(0) as in this case Royalty is not defined

Use pre increment operator

Contract: 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

Recommendation: _computePermitStructHash Function : Change to ++nonces[_owner] instead of nonces[_owner]++

Use Immutable to save gas

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

Recommendation: PERMIT_TYPEHASH : Assigned operations to constant variables are re-evaluated every time.Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Same need to be done for other variables like DOMAIN_TYPEHASH etc

No need to compute length

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

Recommendation: generateMerkleTree Function: Max leaves length allowed is 6. Instead of running the loop till leaves length, its better to check that length is within range

require(leaves<=6,"Incorrect length");

Committed proposal - no need to go forward

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

Recommendation: commit Function : If proposal is already committed then no need for going forward

require(!proposal.isCommited, "Proposal already committed");

No need to compute length

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

Recommendation: getLeafNodes Function: We already know permissions.length is always 5 so array can be revised accordingly

for (uint256 i; i < 5; )

Use local scope

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

Recommendation: withdrawERC721 Function: Line 351-358 for Buyout.sol can be placed in local scope to avoid stack too deep. Same can be done for withdrawERC20

{
(, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
        if (id == 0) revert NotVault(_vault);
        // Reverts if auction state is not successful
        (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
        State required = State.SUCCESS;
        if (current != required) revert InvalidState(required, current);
        // Reverts if caller is not the auction winner
        if (msg.sender != proposer) revert NotWinner();
}