fractionPrice precision can be lost if fractional tokens supply is high enough

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L34-L51

Vulnerability details

Buyout's start() now determine fractional token price by dividing native tokens amount by total supply number. Whenever the supply is high enough the precision can be lost, leading to severe losses to buyout proposer as his staked fractional tokens can be valued at zero this way.

Setting the severity to be high as this is principal funds stealing impact with the only precondition of total the fractional tokens supply to be high enough, which isn't restricted in the system.

Proof of Concept

There is no limitation on the total supply number:

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L34-L51

    function deployVault(
        uint256 _fractionSupply,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        bytes32[] calldata _mintProof
    ) external returns (address vault) {
        bytes32[] memory leafNodes = generateMerkleTree(_modules);
        bytes32 merkleRoot = getRoot(leafNodes);
        vault = IVaultRegistry(registry).create(
            merkleRoot,
            _plugins,
            _selectors
        );
        emit ActiveModules(vault, _modules);

        _mintFractions(vault, msg.sender, _fractionSupply, _mintProof);
    }

Which is reasonable, as the system being base level functionality allows for various granularity of fractional tokens.

In the same time fractionPrice is now determined without precision multiplier, i.e. with straightforward division:

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

uint256 fractionPrice = buyoutPrice / totalSupply;

        // Calculates price of buyout and fractions
        // @dev Reverts with division error if called with total supply of tokens
        uint256 buyoutPrice = (msg.value * 100) /
            (100 - ((depositAmount * 100) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

Once total suypply is big enough this can lead to substantial relative losses for Bob the buyout proposer:

1. Bob owns 90% of totalSupply, which is 1e18, and wants to buyout the rest

2. The only NFT Vault holds is not costly, having market value of 0.98 ETH, Bob's share is valued 0.9 0.98 = 0.882 ETH, and he needs to supply 0.1 0.98 = 0.098 ETH

3. Bob's buyoutPrice will be (0.098e18 100) / (100 - ((0.9e18 100) / 1e18)) = 0.98 ETH

4. Bob's fractionPrice = buyoutPrice / totalSupply = 0.98e18 / 1e18 = 0

5. As start() completes, Alice can now run buyFractions() to buy Bob's fractional tokens, paying 0 for the whole Bob's 90% share

6. Now Alice can wait for the auction to end, the buyout will not be successful as Alice took out the tokens from Vault balance with buyFractions()

7. Then Alice can initiate another buyout, running start() with 90% of tokens and 0.1 ETH msg.value to avoid the precision pitfall Bob experienced

8. Alice's buyoutPrice will be (0.1e18 100) / (100 - ((0.9e18 100) / 1e18)) = 1 ETH

9. Alice's fractionPrice = buyoutPrice / totalSupply = 1e18 / 1e18 = 1

This way Alice buyout will either succeed or someone else would buy her fractional tokens at even higher valuation, yielding at least 0.882 ETH profit. Observing such a possibility Alice can now make a bot that immediately back runs start() transaction similar to Bob's, so initiator will not have the chance to correct the mispricing when observing the buyoutPrice realized.

Recommended Mitigation Steps

Consider adding more precision to the fractionPrice formula and its downstream usage, for example:

+       // @notice Fraction multiplier
+       uint256 public constant FRACTION_MULTI = 1e18;
        ...
+       uint256 fractionPrice = buyoutPrice * FRACTION_MULTI / totalSupply;
-       uint256 fractionPrice = buyoutPrice / totalSupply;

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

+       uint256 ethAmount = fractionPrice * _amount / FRACTION_MULTI;
-       uint256 ethAmount = fractionPrice * _amount;

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

+       if (msg.value != fractionPrice * _amount / FRACTION_MULTI) revert InvalidPayment();
-       if (msg.value != fractionPrice * _amount) revert InvalidPayment();

[stevennevins]

2 - Med

[stevennevins]

Duplicate of #629

[dmitriia]

629 covers zero fractionPrice case only, the issue is bigger than that as any price will lose precision.

I.e. all the findings that describe decimals loss in general aren't duplicates for #629, which is valid, but covers only one corner case.

[HardlyDifficult]

Duping to https://github.com/code-423n4/2022-07-fractional-findings/issues/310