Fractional v2 contest - kenzo's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L111 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L124 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L143 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L157 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L164

Vulnerability details

In Migration, when joining or leaving a migration proposal, Fractional does not check whether the user supplied proposalId and vault match the actual vault that the proposal belongs to. This allows the user to trick the accounting.

Impact

Loss of funds for users. Malicious users can withdraw tokens from proposals which have not been committed yet.

Proof of Concept

Let's say Vault A's FERC1155 token is called TOKEN. Alice has deposited 100 TOKEN in Migration to Vault A on proposal ID 1.

Now Malaclypse creates Vault B with token ERIS as FERC1155 and mints 100 tokens to himself. He then calls Migration's join with amount as 100, Vault B as vault, proposal ID as 1. The function will get ERIS as the token to deposit. It will pull the ERIS from Mal. And now for the problem - it will set the following variable:

        userProposalFractions[_proposalId][msg.sender] += _amount;

Notice that this does not correspond to the vault number.

Now, Mal will call the leave function, this time with Vault A address and proposal ID 1. The function will get the token to send from the vault as TOKEN. It will get the amount to withdraw from userProposalFractions[_proposalId][msg.sender], which as we saw previously will be 100. It will deduct this amount from migrationInfo[_vault][_proposalId], which won't revert as Alice deposited 100 to this vault and proposal. And finally it will send 100 TOKENs to Mal - although he deposited ERIS. Mal received Alice's valuable tokens.

Recommended Mitigation Steps

I think that one option would be to save for each proposal which vault it corresponds to. Then you can verify that user supplies a matching vault-proposal pair, or he can even just supply proposal and the contract will get the vault from that. Another solution would be to have userProposalFractions save the relevant vault also, not just a general proposal id.

Lines of code

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

Vulnerability details

The fractionPrice is saved as buyoutPrice / totalSupply. This leads to loss of precision, and at worst case, can realistically round down to 0.

Impact

Alice might initiate a buyout with legitimate shares and ETH deposited, but the contract will round down the fractionPrice to 0, therefore other users can buy Alice's shares for zero cost.

Proof of Concept

When starting a buyout, Buyout rightfully calculates the buyoutPrice according to the proposer's sent ETH and shares, and then saves the fractionPrice to be buyoutPrice / totalSupply. But this can realistically round down to 0.

For example let's look at Fractional's current popular PUNKS vault. It has 1e8 fractions, and each fractions 1e18 decimals, making a totalSupply of 1e8*1e18. (This can also be verified by querying the PUNKS token contract). The current implied valuation is 4600 ETH, meaning 4600e18.

Let's say Alice initiates a buyout such that the buyout price would be 50000e18 - quite a generous proposal. But since totalSupply is 1e26, then fractionPrice (buyoutPrice / totalSupply) would be saved as 5e24/1e26 = 0.

Therefore, since fractionPrice is 0, and it is the value used to calculate how much other users need to pay for Alice's tokens:

        if (msg.value != fractionPrice * _amount) revert InvalidPayment();

The users can pay 0 and buy Alice's tokens for free.

Recommended Mitigation Steps

Instead of saving fractionPrice, save the buyoutPrice. The total supply is already saved in the proposal. Then throughout the contract, instead of using fractionPrice, use buyoutPrice * _amount / totalSupply. This will prevent the loss of precision. Additionally, to prevent users from buying miniscule amounts of shares for free, you should probably require that this payment calculation will be bigger than 0.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L308 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L321 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L312 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L325

Vulnerability details

When a user calls withdrawContribution, it will try to send him back his original contribution for the proposal. But if the proposal has been committed, and other users have interacted with the buyout, Migration will receive back a different amount of ETH and tokens. Therefore it shouldn't send the user back his original contribution, but should send whatever his share is of whatever was received back from Buyout.

Impact

Loss of funds for users. Some users might not be able to withdraw their contribution at all, and other users might withdraw funds that belong to other users. (This can also be done as a purposeful attack.)

Proof of Concept

A summary is described at the top.

It's probably not needed, but the here's the flow in detail. When a user joins a proposal, Migration saves his contribution:

        userProposalEth[_proposalId][msg.sender] += msg.value;
        userProposalFractions[_proposalId][msg.sender] += _amount;

Later when the user would want to withdraw his contribution from a failed migration, Migration would refer to these same variables to decide how much to send to the user:

        uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
        IFERC1155(token).safeTransferFrom(address(this), msg.sender, id, userFractions, "");
        uint256 userEth = userProposalEth[_proposalId][msg.sender];
        payable(msg.sender).transfer(userEth);

But if the proposal was committed, and other users interacted with the buyout, then the amount of ETH and tokens that Buyout sends back is not the same contribution. For example, if another user called buyFractions for the buyout, it will decrease the amount of tokens in the pool:

        IERC1155(token).safeTransferFrom(address(this), msg.sender, id, _amount, "");

And when the proposal will end, if it has failed, Buyout will send back to Migration the amount of tokens in the pool:

        uint256 tokenBalance = IERC1155(token).balanceOf(address(this), id);
        ...
        IERC1155(token).safeTransferFrom(address(this), proposer, id, tokenBalance, "");

(**Same will happen for the ETH amount)

Therefore, Migration will receive back less tokens than the original contribution was. When the user will try to call withdrawContribution to withdraw his contribution from the pool, Migration would try to send the user's original contribution. But there's a deficit of that. If other users have contributed the same token, then it will transfer their tokens to the user. If not, then the withdrawal will simply revert for insufficient balance.

Recommended Mitigation Steps

I am not sure, but I think that the correct solution would be that upon a failed proposal's end, there should be a hook call from Buyout to the proposer - in our situation, Migration. Migration would then see(/receive as parameter) how much ETH/tokens were received, and update the proposal with the change needed. eg. send to each user 0.5 his tokens and 1.5 his ETH. In another issue I submitted, "User can't withdraw assets from failed migration if another buyout is going on/succeeded", I described for a different reason why such a callback to Migration might be needed. Please see there for more implementation suggestion. I think this issue shows that indeed it is needed.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L268 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L270

Vulnerability details

When cashing out a successful buyout, the cash function takes the ETH amount to be considered from buyoutInfo, but does not update this buyoutInfo ETH balance after a user has cashed out.

Impact

Wrong amounts sent to users. Some users would not be able to cash out at all.

Proof of Concept

The buyoutInfo contains the ETH balance relevant for the buyout and is being increased or decreased when users buy or sell fractions.

After a successful buyout, when users cash out their fractions, the function takes the ETH balance to be distributed from buyoutInfo (ethBalance), and calculates the amount of ETH to send to the user like so:

        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
        uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);
        _sendEthOr23Weth(msg.sender, buyoutShare);

Note that the buyoutInfo.ethBalance is not being updated after this redemption. Therefore the wrong ETH balance would be considered for subsequent redememptions.

For illustration consider the following case: Alice has 5 vault tokens and Bob has 5 vault tokens. Vault's total supply is 10. The buyout has 23 ETH deposited in buyoutInfo. Alice cashes out. According to the above calculation:

totalSupply = 10
buyoutShare = (5*23e18)/(5+5) = 11.5e18

Therefore, Alice will be sent 11.5 ETH.

Now Bob tries to cash out. ethBalance has not been updated and is still 23 ETH. This time the calculation would be:

totalSupply = 5
buyoutShare = (5*23e18)/(0+5) = 23e18

But the contract does not have 23 ETH anymore. The transfer would fail and Bob would not receive his shares. Sorry, Bob. We will not forget. His name was Robert Paulsen.

Recommended Mitigation Steps

I believe you need to decrease the buyoutInfo.ethBalance by buyoutShare upon redemption.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L341 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L365 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L391

Vulnerability details

If some migration proposal has succeeded, A childish user can call the asset migration functions with an unexisting proposal as parameter, and the migration functions would continue to send the vault assets to the 0 address.

Also, if somebody calls these functions with the right proposal but before settleVault has been called, the assets would be sent to the 0 address.

Impact

Total loss of assets for the vault.

Proof of Concept

The various migration functions - migrateVaultERC20, migrateVaultERC721, migrateVaultERC1155 - take the assets receiver address to be:

        address newVault = migrationInfo[_vault][_proposalId].newVault;

So if a migration has succeeded, and a user passes an unexisting/unsettled proposal, the newVault would be 0. So Migration would try to send the tokens to the 0 address. Buyout does not check whether the recipient is the 0 address. And some token implementations, eg. SolmateERC20, allow transferring to the 0 address. Therefore, the assets would be transferred to 0 and lost.

Recommended Mitigation Steps

Revert in these functions if newVault == address(0).

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L150

Vulnerability details

Migration only allows a user to leave a proposal if there's no live/successful buyout for the vault. This is basically similar problem to my other issue, "User can't withdraw assets from failed migration if another buyout is going on/succeeded", but I'm mentioning it also here in case they will be judged as separate issues.

Impact

Users will not be able to leave proposals; their funds will be locked.

Proof of Concept

Let's say there are 2 competing migration proposals for a certain vault. Alice joined proposal A, but then proposal B is committed, and is successful. The state of the vault's buyout will always be SUCCESS. Therefore, Alice will never be able to call leave to withdraw her contribution, as `leave will revert if the buyout's state is not INACTIVE.

Recommended Mitigation Steps

I think the solution is the same one I proposed in "User can't withdraw assets from failed migration if another buyout is going on/succeeded", please see there for full details. In short: save the lastProposal submitted for each vault, and allow users to leave a proposal only if it is not the lastProposal. This var will need to be cleaned upon every failed migration. Please see in the issue mentioned above.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L433

Vulnerability details

When a migration proposal has succeeded, a user can call migrateFractions with an old failed proposal, and the fractions will be settled according to the old proposal.

Impact

Migration will be wrongly settled, Malicious user can mint all the new vault's tokens to himself, Thereby he will be able to initiate a buyout to a vault that will allow him to withdraw all the assets.

Proof of Concept

The issue is similar in nature to another issue I submitted: "Migration: unsuccessful proposals can be settled when another proposal has succeeded". This time in short, migrateFractions does not check whether the user-supplied proposalId is the latest proposal that has actually succeeded. Therefore, after a legitimate migration buyout has succeeded, a malicious user can call migrateFractions with an earlier failed proposal that he was the only contributor of, and thereby mint all the new vault's tokens to himself. As he now totally controls the new vault, he can call Buyout's redeem on the new vault and transfer all assets to himself.

Recommended Mitigation Steps

Basically same as the solution I proposed for the other issue mentioned above: I believe Migration should save the last proposal submitted for each vault, and only allow settling the fractions for this winning proposal. For more implementation details on this new variable, please see my other issue: "User can't withdraw assets from failed migration if another buyout is going on/succeeded".

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L220

Vulnerability details

In settleVault, which creates a new vault for a successful migration, Migration takes as user-supplied argument the proposal to settle, and doesn't check whether the proposal that succeeded is the proposal that the user supplied. Therefore a user can supply an older proposal that failed, and Migration would migrate the vault to that proposal.

Impact

Total loss of assets of the vault - the user can supply his own proposal which allows him to withdraw all the assets.

Proof of Concept

Let's say Malaclypse issues a proposal to migrate some vault to his own vault which just allows him to withdraw all the assets. The proposal is committed to Buyout, but then the buyout fails, and everybody at Discord laughs at his pathetic attempt of a takeover.

Later, Winfred issues a proposal which everybody agrees to. The migration proposal is a success. But Malaclypse issues a transaction exactly when the buyout rejection period ends; he ends the buyout using end, and then calls settleVault with his own proposal.

Now settleVault checks whether Mal's proposal is commited (it is), whether the current buyout has succeeded (it has), and whether Mal's propsal was already settled (it wasn't). Therefore, all the checks would succeed, and the function would continue to create a new vault from Mal's proposal. Mal will then migrate the vault's assets to his new vault. Finally, Mal goes back to the Discord which previously mocked him, and asks them, "who's laughing now?".

** Note that in settleVault, even if the rightful proposal has already been settled, it can be called again with this previous malicious proposal. Then the migration to the malicious vault can continue.

Recommended Mitigation Steps

I suggest that Migration should save the last buyout proposal ID submitted for each vault, and will only allow to settleVault with the last proposal ID (which has been accepted). But it will also need to delete this variable if the proposal has been rejected. For full details, please see my other issue, "User can't withdraw assets from failed migration if another buyout is going on/succeeded".

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L141

Vulnerability details

The leave function allows to leave a proposal even if the proposal has been committed and failed. This makes it a (probably unintended) duplicate functionality of withdrawContributions, which is the function that should be used to withdraw failed contributions.

Impact

User assets might be lost: When withdrawing assets from a failed migration, users should get back a different amount of assets, according to the buyout auction result. (I detailed this in another issue - "Migration::withdrawContribution falsely assumes that user should get exactly his original contribution back"). But when withdrawing assets from a proposal that has not been committed, users should get back their original amount of assets, as that has not changed. Therefore, if leave does not check if the proposal has been committed, users could call leave instead of withdrawContribution and get back a different amounts of assets than they deserve, on the expense of other users.

Proof of Concept

The leave function does not check anywhere whether proposal.isCommited == true. Therefore, if a user calls it after a proposal has been committed and failed, it will continue to send him his original contribution back, instead of sending him the adjusted amount that has been returned from Buyout.

Recommended Mitigation Steps

Revert in leave if proposal.isCommited == true. You might be also able to merge the functionality of leave and withdrawContribution, but that depends on how you will implement the fix for withdrawContribution.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L40 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L73 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L86

Vulnerability details

Even when a buyout is going on, the vault's owner can execute any commands whatsoever on the vault. This includes for example withdrawing the vault's assets.

Impact

The buyout/migration modules are not fully effective. Every buyout proposer has to take the risk that his buyout might end up being worth nothing.

A governance(/owner/multisig) can "walk back" on the promised buyout if the governance does not like the results. At it's current form, this might cause loss to the buyout proposer, as his offer might be accepted, his ETH gone, but no assets will remain in the vault.

Proof of Concept

When a buyout is proposed, there is no mechanism in place to stop general Vault commands from being executed. Therefore the owner can install a plugin or module that upon execution will move all the vault assets to a different address. There's no option to cancel a buyout. Therefore, a buyout may succeed, even though the vault is now worthless and has no assets. The proposer has simply lost his assets.

Recommended Mitigation Steps

Consider adding an option to the vault/buyout that upon buyout proposal, the vault's assets would be moved to a different vault/Buyout itself - a location which only Buyout can access. This way, the Vault owner would not be able to execute commands that can invalidate the buyout process, such as removing assets from the vault, or updating the vault's permissions (Merkle root). Then when the buyout is finished, if it is successful, Buyout would allow the proposer to withdraw the assets, and if it has failed, Buyout would return the assets to the vault.

Another option for mitigation would be to limit Vault modules/plugins execution when a Buyout is in place, but that might be too restrictive.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L57

Vulnerability details

A user can start a buyout while depositing 0 tokens and sending 1 wei of ether. No other buyout could then be started for 4 days (rejection period).

Impact

Seems it's too easy to DOS the buyout mechanism. In case of governance disagreements this might cause difficulties and delays.

For example, consider something like the recent Rari-Tribe FUSE hack repayment scenario: Gov would like to redeem/move certain tokens to repay users for hack. It requires a vault migration to do so. Some users do not want this happen. They can start their own buyout (with almost 0 investment) and cause delay of 4 days. Meanwhile, the tokens that the gov wishes to redeem might drop in value.

Proof of Concept

A buyout proposer can't deposit 0 ether, but he can deposit 1 wei. He needn't even own any FERC1155s as there's no check that he actually deposited any. The fractionPrice can be set as 0. Therefore, it is trivial to start a buyout.

After that, the auction can only be ended after the rejection period has finished.

Therefore any user can delay and hinder buyouts and migrations.

Recommended Mitigation Steps

I think you should add an option to end function: if the buyout has 0 vault tokens, the buyout can be ended immediately. This will easily and fairly mitigate the time-delay factor of the griefing. If a propser has sent 0 tokens, the buyout can be simply ended, and if he set an unattractive fraction price, likewise somebody can just buy him out and end the proposal.

There are other options available for preventing griefing, such as requiring a minimum amount of deposit tokens, but that is a different issue I believe. Here I just intend to point out and remedy the seemingly-unnecessary time delay introduced by malicious/unattractive proposals.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L16 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L17

Vulnerability details

As per the docs and the comments, BaseVault is supposed to have a Buyout mechanism. But it doesn't have one by default.

Impact

Users may deploy a vault using BaseVault, thinking that it has a Buyout mechanism - but it hasn't. Therefore users' assets would be forever locked in the vault. Since this locks user assets, I believe this can be considered a high severity issue.

Proof of Concept

The BaseVault comment says:

/// @notice Protoform contract for vault deployments with a fixed supply and buyout mechanism

However, we see that the vault does not inherit Buyout:

contract BaseVault is IBaseVault, MerkleBase, Minter, Multicall {

Nor does it add Buyout to the list of modules upon vault deployment:

    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);
    }

Therefore, users would have to supply the Buyout module themselves - otherwise their assets would be forever locked.

As the whole idea of a Protoform is to be a template for common vault usages, and BaseVault is mentioned to be a template for vaults with fixed supply and buyout mechanism, Not actually having by default a buyout mechanism seems like a big omission which users might not know.

Recommended Mitigation Steps

Upon deployment of BaseVault, save the Buyout module address, and add it to the list of modules in deployVault.