Fractional v2 contest - cryptphi's results

Lines of code

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

Vulnerability details

Impact

The ERC 1155 standard requires that smart contracts must implement onERC1155Received and onERC1155BatchReceived to accept transfers. This means that on any token received, code run on the receiving smart contract.

An attacker can call buyFractions() using a malicious contract which calls back into the buyFractions() function each time token is received, while making buyoutInfo[_vault].ethBalance state updated once. With this, the attacker can then go back to sell their fractional token and withdraw their eth back while leaving some users unable to sell their fractional tokens due to the buyoutInfo[_vault].ethBalance mismatch. The incorrect ethBalance would also affect the cash() and end() functions .

Proof of Concept

1. Attacker.sol contract is used to call Buyout.buyFractions() to buy 2 fractional tokens paying equivalent eth to the Buyout contract.
2. On receiving the ERC1155 token, Attacker contract calls back into buyFractions().
3. buyoutInfo[_vault].ethBalance state is not updated while Buyout contract receives eth.
4. While calling back into buyFractions() a few times, there will be an incorrect ethBalance.
5. Attacker sells their fractional tokens and gets back eth
6. Users call cash() after successful Buyout
7. Due to the incorrect ethBalance , buyoutShare value will be lesser and users receive an incorrect amount of eth.

Tools Used

Manual review

Recommended Mitigation Steps

Some reentrancy guards may be adequate but at same time ensuring contract is not affected by cross-function reentrancy.

Lines of code

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

Vulnerability details

Impact

The use of .transfer() in on an address payable may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender is a smart contract. Funds can potentially be lost if;

1. The smart contract fails to implement the payable fallback function
2. The fallback function uses more than 2300 gas units

The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the swap.

A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found in https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual review

Recommended Mitigation Steps

Using call() instead of transfer() is recommended

1. Missing zero address check The following functions are missing a zero address check, which may cause vaults ownership to become address(0).

**Occurrences in: https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L217-L225 https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultFactory.sol#L73 https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultRegistry.sol#L74

2. Missing input validation on array lengths The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L73-L82 https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultRegistry.sol#L165-L177 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L413-L445 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L72-L99 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L410-L428 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L34-L51 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L58-L70 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L77-L89 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L98-L117 https://github.com/code-423n4/2022-07-fractional/blob/main/src/references/TransferReference.sol#L61-L69 https://github.com/code-423n4/2022-07-fractional/blob/main/src/targets/Transfer.sol#L474 https://github.com/code-423n4/2022-07-fractional/blob/main/src/utils/NFTReceiver.sol#L33-L41

Gas Optimisation

1. Use calldata instead of memory for string _uri https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L68

2. The following variables should be cached to save gas _vault in Buyout.cash() _vault in Buyout.redeem() _vault in Buyout.withdrawERC20() _vault in Buyout.withdrawERC721() _vault in Buyout.withdrawERC1155() _vault in Buyout.batchWithdrawERC1155() _vault in Migration.migrateFractions() _proposalId in Migration.migrateFractions() _vault in Migration.withdrawContribution() _proposalId in Migration.withdrawContribution() _proposalId in Migration.settleFractions() _proposalId in Migration.settleVault() _vault in Migration.commit() _proposalId in Migration.commit() _vault in Migration.leave() _proposalId in Migration.leave() _vault in Migration.join() _proposalId in Migration.join() _amount in Migration.join()