Fractional v2 contest - Kumpa's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L149

Vulnerability details

buyFractions does not follow check effects interaction, opening up reentrancy attack vector. After ERC1155 is sent to a malicious contract and calls INFTReceiver().onERC1155Received, an attacker could configure the receiving address to buyFractions again to bypass buyoutInfo[_vault].ethBalance += msg.value;.

Even though the attacker does not directly benefit from this attack, the attack could distort the value of buyoutInfo[_vault].ethBalance, causing certain amount of ether to be frozen in the vault.

Proof of concept

1.An attacker creates a malicious contract with function onERC1155Received() that trigger buyFractions after called

2.An attacker initially calls buyFractions through a malicious contract and exchange ethers for fractionTokens

3.When IERC1155.safeTransferFrom() occurs, apart from updating fractionTokens in the malicious contract, it will also call onERC1155Received which initiate reentrancy loop

4.The malicious contract will keep rebuying fractionTokens and buyoutInfo[_vault].ethBalance will only update the final msg.value that get sent while prior msg.value is bypassed

5.After the aunction ends and buyoutInfo[_vault].state = State.SUCCESS;. An attacker could quickly cash to turn their fractionTokens to ethers.

6.Other users that cash later may not be able to convert their fractionTokens into ethers because the supply of fractionsToken will be more than ethBalance due to reentrancy buying that bypass the accounting of eth, causing some ethers to be frozen.

Mitigations

1.Use nonreentrant from openzeppelin to prevent reentrancy attack

2.Update ethBalance before calling external contract to follow check effects interaction pattern

Lines of code

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

Vulnerability details

Impact

This could render the vault unfunctioning when the current owner accidently set address(0) as the new owner. After function is called, noone will be able to relinquish the action and the entire contract will be ungovernable.

Proof of concept

Mitigations

Add require (_newOwner != address(0), "address does not exist") to ensure that address(0) will not be input.