Open code423n4 opened 2 years ago
Duplicate of #148
This is a valid suggestion to consider, improving robustness for future modules. Lowering risk and merging with the warden's QA report #524
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).
Fair point. I'll reset this to Medium. Thanks
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 🙏
Hi Just wanna address a few points here.
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).
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.
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.
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 thefractionPrice
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 thefractionPrice
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
Proof of Concept
Consider the following scenario
Here's a test (added to the
test/Buyout.t.sol
file) demonstrating this scenario (test passes = the bug exists).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: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.
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;
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 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 { );
@@ -272,6 +287,18 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit { emit Cash(_vault, msg.sender, buyoutShare); }