Buyout's buyFractions can be called multiple time reusing the same msg.value with Multicall

[code423n4]

Lines of code

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

Vulnerability details

Passing multiple buyFractions() calls to Multicall's multicall() will use the same msg.value many times. This will inflate his contribution without real fund transfers with the corresponding fund loss for the system.

Proof of Concept

Buyout uses Multicall:

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

contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {

And Buyout's buyFractions() uses msg.value:

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

        // Updates ether balance of pool
        buyoutInfo[_vault].ethBalance += msg.value;

Buyout's start() also uses msg.value, but is not straightforwardly affected as it performs state change, State.INACTIVE -> State.LIVE, and State.INACTIVE is required:

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

        // 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;

        // Sets info mapping of the vault address to auction struct
        buyoutInfo[_vault] = Auction(
            block.timestamp,
            msg.sender,
            State.LIVE,
            fractionPrice,
            msg.value,
            totalSupply
        );

Also, Migration's join can by called with Multicall:

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

        // Gets the migration proposal for the given ID
        Proposal storage proposal = migrationInfo[_vault][_proposalId];
        // Updates ether balances of the proposal and caller
        proposal.totalEth += msg.value;
        userProposalEth[_proposalId][msg.sender] += msg.value;

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

contract Migration is
    IMigration,
    MerkleBase,
    Minter,
    Multicall,

Recommended Mitigation Steps

If Multicall is introduced as a convenience mechanics only consider removing this Multicall inheritance as it becomes unsafe for any system dealing with native token funds due to the reuse of msg.value. As this provides a way to empty the Vault holdings, in this case security considerations out weight user convenience ones.

[KenzoAgada]

Actually this multicall is not payable, so this issue with msg.value is not present.

[mehtaculous]

0 (Not Bug)

Multicall is not marked as payable and so this issue would never occur

[HardlyDifficult]

Closing as invalid since the function is not payable.