[Buyout module] Fraction price is not updated when total supply changes

[code423n4]

Lines of code

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

Vulnerability details

In the buyout module when a buyout starts - the module stores the fractionPrice, and when a user wants to buy/sell fractions the fractionPrice is loaded from storage and based on that the module determines the price of the fractions.
The issue here is that the total supply might change between the time the buyout start till the buy/sell time, and the fractionPrice stored in the module might not represent the real price anymore.

Currently there are no module that mint/burn supply at the time of buyout, but considering that Fractional is an extendible platform - Fractional might add one or a user might create his own module and create a vault with it.
An example of an innocent module that can change the total supply - a split module, this hypothetical module may allow splitting a coin (multiplying the balance of all users by some factor, based on a vote by the holders, the same way QuickSwap did at March)).
If that module is used in the middle of the buyout, that fraction price would still be based on the old supply.

Impact

• Buyout proposer can end up paying the entire buyout price, but ending up with only part of the vault.
• Users may end up buying fractions for more than they're really worth (if they're unaware of the change in total supply).
• Users may end up getting a lower price than intended while selling their fractions (in case of a burn).

Proof of Concept

Consider the following scenario

• Alice creates a vault with a 'split' module
• Bob starts a buyout for the price of 1 ETH
• Alice runs the split modules twice (making the total supply 4 times the original supply) and then sells 25% of her fractions.
• Bob lost his 1 ETH and got in exchange only 25% of the fractions.

Here's a test (added to the test/Buyout.t.sol file) demonstrating this scenario (test passes = the bug exists).

    function testSplit_bug() public {
        initializeBuyout(alice, bob, TOTAL_SUPPLY, 0, true);

        // Bob proposes a buyout for 1 ether for the entire vault
        uint buyoutPrice = 1 ether;
        bob.buyoutModule.start{value: buyoutPrice}(vault);

        // simulate a x4 split
        // Alice is the only holder so we need to multiply only her balance x4
        bytes memory data = abi.encodeCall(
            Supply.mint,
            (alice.addr, TOTAL_SUPPLY * 3)
        );
        address supply = baseVault.supply();
        Vault(payable(vault)).execute(supply, data, new bytes32[](0));

        // Alice now sells only 1/4 of the total supply 
        // (TOTAL_SUPPLY is now 1/4 of the actual total supply)
        alice.buyoutModule.sellFractions(vault, TOTAL_SUPPLY);

        // Alice got 1 ETH and still holds 3/4 of the vault's fractions
        assertEq(getETHBalance(alice.addr), buyoutPrice + INITIAL_BALANCE);
        assertEq(getFractionBalance(alice.addr), TOTAL_SUPPLY * 3);

    }

Trying to create a proof for minting was too much time-consuming, so I just disabled the proof check in Vault.execute in order to simulate the split:

        // if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
        //     if (msg.sender != owner)
        //         revert NotAuthorized(msg.sender, _target, selector);
        // }

Tools Used

Foundry

Recommended Mitigation Steps

Calculate fraction price at the time of buy/sell according to the current total supply:
(Disclosure: this is based on a solution I made for a different bug)

• This can still cause an issue if a user is unaware of the new fraction price, and will be selling his fractions for less than expected. Therefore, you'd might want to revert if the total supply has changed, while adding functionality to update the lastTotalSupply - this way there's an event notifying about the fraction-price change before the user buys/sells.

diff --git a/src/interfaces/IBuyout.sol b/src/interfaces/IBuyout.sol
index 0e1c9eb..79beb71 100644
--- a/src/interfaces/IBuyout.sol
+++ b/src/interfaces/IBuyout.sol
@@ -20,7 +20,7 @@ struct Auction {
     // Enum state of the buyout auction
     State state;
     // Price of fractional tokens
-    uint256 fractionPrice;
+    uint256 buyoutPrice;
     // Balance of ether in buyout pool
     uint256 ethBalance;
     // Total supply recorded before a buyout started
diff --git a/src/modules/Buyout.sol b/src/modules/Buyout.sol
index 1557233..d9a6935 100644
--- a/src/modules/Buyout.sol
+++ b/src/modules/Buyout.sol
@@ -63,10 +63,13 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         );
         if (id == 0) revert NotVault(_vault);
         // Reverts if auction state is not inactive
-        (, , State current, , , ) = this.buyoutInfo(_vault);
+        (, , State current, , ,uint256 lastTotalSupply) = this.buyoutInfo(_vault);
         State required = State.INACTIVE;
         if (current != required) revert InvalidState(required, current);
 
+        if(totalSupply != lastTotalSupply){
+            // emit event / revert / whatever 
+        }
         // Gets total supply of fractional tokens for the vault
         uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
         // Gets total balance of fractional tokens owned by caller
@@ -85,14 +88,14 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         // @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;
+        uint256 fractionEstimatedPrice = buyoutPrice / totalSupply;
 
         // Sets info mapping of the vault address to auction struct
         buyoutInfo[_vault] = Auction(
             block.timestamp,
             msg.sender,
             State.LIVE,
-            fractionPrice,
+            buyoutPrice,
             msg.value,
             totalSupply
         );
@@ -102,7 +105,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
             msg.sender,
             block.timestamp,
             buyoutPrice,
-            fractionPrice
+            fractionEstimatedPrice
         );
     }
 
@@ -115,8 +118,9 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
             _vault
         );
         if (id == 0) revert NotVault(_vault);
-        (uint256 startTime, , State current, uint256 fractionPrice, , ) = this
+        (uint256 startTime, , State current, uint256 buyoutPrice, ,  ) = this
             .buyoutInfo(_vault);
+        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
         // Reverts if auction state is not live
         State required = State.LIVE;
         if (current != required) revert InvalidState(required, current);
@@ -135,7 +139,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         );
 
         // Updates ether balance of pool
-        uint256 ethAmount = fractionPrice * _amount;
+        uint256 ethAmount = buyoutPrice * _amount / totalSupply;
         buyoutInfo[_vault].ethBalance -= ethAmount;
         // Transfers ether amount to caller
         _sendEthOrWeth(msg.sender, ethAmount);
@@ -153,16 +157,27 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         );
         if (id == 0) revert NotVault(_vault);
         // Reverts if auction state is not live
-        (uint256 startTime, , State current, uint256 fractionPrice, , ) = this
+        (uint256 startTime, , State current, uint256 buyoutPrice, , uint256 lastTotalSupply ) = this
             .buyoutInfo(_vault);
+        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+        
+        if(totalSupply != lastTotalSupply){
+            // emit event / revert / whatever 
+        }
+
         State required = State.LIVE;
         if (current != required) revert InvalidState(required, current);
         // Reverts if current time is greater than end time of rejection period
         uint256 endTime = startTime + REJECTION_PERIOD;
         if (block.timestamp > endTime)
             revert TimeExpired(block.timestamp, endTime);
+
+        uint256 price = (buyoutPrice * _amount) / totalSupply;
+        if (price * totalSupply < buyoutPrice * _amount){
+            price++;
+        }
         // Reverts if payment amount does not equal price of fractional amount
-        if (msg.value != fractionPrice * _amount) revert InvalidPayment();
+        if (msg.value != price) revert InvalidPayment();
 

@@ -272,6 +287,18 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         emit Cash(_vault, msg.sender, buyoutShare);
     }
 
+    function updateSupply(address _vault) external{
+        (, , , uint256 buyoutPrice, , uint256 lastTotalSupply ) = this.buyoutInfo(_vault);
+
+        uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(_vault);
+        uint256 newEstimatedFractionPrice = buyoutPrice / newTotalSupply;
+        if(newTotalSupply == lastTotalSupply){
+            revert SupplyHasntChanged();
+        }
+        this.buyoutInfo(_vault).lastTotalSupply = newTotalSupply; 
+        emit TotalSupplyChanged(lastTotalSupply, newTotalSupply, newEstimatedFractionPrice);
+    }

[stevennevins]

Duplicate of #148

[HardlyDifficult]

This is a valid suggestion to consider, improving robustness for future modules. Lowering risk and merging with the warden's QA report #524

[0xA5DF]

Reading Fractional's docs, it seems that they intend the vaults to use not only their modules, but also from other sources as long as they're trusted:

Additionally, users should only interact with Vaults that have been deployed using modules that they trust, since a malicious actor could deploy a Vault with malicious modules.

An innocent user or an attacker can be creating a split module, even getting it reviewed or audited and then creating a vault with it.
Users would trust the vault, and when the bug is exploited it'd be the Bouyout module responsibility since it's the one that contains the bug (if your platform is intended to be extendable, then you should take into account any normal behavior that those extensions might have).

[HardlyDifficult]

Fair point. I'll reset this to Medium. Thanks

[stevennevins]

Just to add, we're not certifying that the Buyout is safe in every context that it could be used in. In that statement we were trying to indicate that you can add modules outside of our curated set, but you would need to be aware of the trust assumptions with regards to both the individual module as well as their composition with others ie rapid inflationary mechanisms and a buyout. I recognize that we could have better handled the case of fraction supply changes during a buyout but inflation was outside of our initial scope for our curated launch. Thank you for reviewing our protocol and providing feedback it's greatly appreciated 🙏

[0xA5DF]

Hi
Just wanna address a few points here.

Scope:

I don't think it's fair towards wardens to exclude whatever wasn't explicitly excluded in the contest description that was given at the beginning of the contest (I don't mean to explicitly address that specific issue, but to have the exclusion clearly inferred from the given contest description).

Responsibility:

While sponsors input and the way they view the platform is important, I think what matters the most is the way the users would view it with the given docs and the trust they'll loose in the platform in case of an attack.
Since it isn't mentioned anywhere that the modules assume total supply didn't change and that new modules should specifically look into the existing modules, I believe the platform would bare at least some responsibility in case of an attack. Since the platform is intended to be flexible and a supply change is a very normal behavior which should be taken into account.

Severity:

I'm not sure if C4 is going by OWASP severity assessment model (last reports do mention it, the docs repo does too but the docs website doesn't seem to mention it), but it seems like it's going along the lines of it.
So it's worth mentioning that the impact of this issue is high - loss of assets, so under the OWASP model as long as the likelihood of it happening (under normal user behavior) is not negligible I think this should be considered medium.