Fractional v2 contest - 0xsanson's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/VaultFactory.sol#L20-L22 https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L24-L29

Vulnerability details

Impact

Every Vault is a proxy of the same implementation contract. This implementation is deployed from VaultFactory but never initialized.

/// @notice Initializes implementation contract
constructor() {
    implementation = address(new Vault());
}

Someone can call init() in the implementation and become the owner.

/// @dev Initializes nonce and proxy owner
function init() external {
    if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
    nonce = 1;
    owner = msg.sender;
    emit TransferOwnership(address(0), msg.sender);
}

Having total control over the contract, they can delegatecall to a selfdestruct. This basically blocks every Vault functionality.

Proof of Concept

1. Alice calls init() on the Vault's implementation;
2. she installs a plugin that has a selfdestruct function;
3. she executes that function (using fallback);

Now the implementation contract is destroyed, leading to the loss of functionality of all Vaults.

Recommended Mitigation Steps

init() the implementation after creating it.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L206-L213 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L72-L82

Vulnerability details

Impact

The Migration contract holds all fractional token balance of all proposals.

Let's suppose a single vault has multiple proposals going on, and one gets committed. If the target price is satisfied, then a buyout starts

// Checks if the current price is greater than target price of the proposal
if (currentPrice > proposal.targetPrice) {
    // Sets token approval to the buyout contract
    IFERC1155(token).setApprovalFor(address(buyout), id, true);
    // Starts the buyout process
    IBuyout(buyout).start{value: proposal.totalEth}(_vault);
    proposal.isCommited = true;
    started = true;
}

Inside Buyout.start, all balance of the contract is taken:

// Gets total balance of fractional tokens owned by caller
uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);

// Transfers fractional tokens into the buyout pool
IERC1155(token).safeTransferFrom(
    msg.sender,
    address(this),
    id,
    depositAmount,
    ""
);

Basically the buyout starts using also the fractional balance allocated for other proposals.

Proof of Concept

Alice starts a malicious proposal (id = 1). After a while a good proposal (id = 2) picks up, and users joins it by depositing their tokens.

Alice can call commit for proposal 1, starting the buyout (suppose her target price was very low). Since the buyout starts with also the balance for proposal 2, it's possible that her malicious proposal passes the buyout and is settled.

Recommended Mitigation Steps

Buyout.start(...) should have an input argument for the token balance to transfer. Migration.commit(...) should pass migrationInfo[_vault][_proposalId].totalFractions.

Lines of code

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

Vulnerability details

Impact

When a buyout is successful, token owners can cash out their fractional tokens for ETH. The amount of ETH cashed out (buyoutShare) is calculated like this (L268):

uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);

with tokenBalance the user's fractional token amounts getting burned, totalSupply the total supply after the burn, ethBalance the buyoutInfo.ethBalance.

During the function, buyoutInfo isn't changed.

The next user that calls cash will have totalSupply lower compared to the first user, meaning that they'll get a bigger share. This leads to certain steal of funds as described below.

Proof of Concept

Let's consider the buyout for a vault just ended successfully, with a total 10 ETH raised (buyoutInfo.ethBalance). The total supply of fractionals is currently 1000, Alice and Bob both have 500 tokens.

Alice can call cash. Her share is 10ETH * 500/1000 = 5 ETH. The total supply goes down to 500. Bob calls cash next. His share is 10ETH * 500/500 = 10 ETH, when it was supposed to be equal to 5 ETH.

Since all ETH of all vault's buyouts is kept in the Buyout contract, it's easy to see that it can all be drained using this method.

For example, Alice can make a vault with an irrelevant NFT. She can then start a buyout for it and at end get out more ETH than what she deposited (in the Alice+Bob example they got 15 ETH out of a deposit of 10 ETH). By repeating the process, or by having a large initial capital, all funds can be drained.

Another way for Alice to game the system is the following:

1. distribute her tokens in 5 different wallets, 100 each;
2. cash from all of them:
   • first share is 10ETH * 100/1000
   • second share is 10ETH * 100/900
   • ...
3. the total share taken out this way is 10ETH * (100/1000 + 100/900 + ... + 100/600) ~= 6.45 ETH, compared to the fair 5 ETH.

Recommended Mitigation Steps

Change the share formula to use an old totalSupply. Also write more extensive tests to catch these errors.

Lines of code

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

Vulnerability details

Impact

There are two functions that users can use to withdraw deposited ether/tokens from a joined proposal: leave and withdrawContribution.

leave correctly decreases the total contribution from a proposal alongside the user's one:

// Updates fraction balances of the proposal and caller
uint256 amount = userProposalFractions[_proposalId][msg.sender];
proposal.totalFractions -= amount;
userProposalFractions[_proposalId][msg.sender] = 0;
// Updates ether balances of the proposal and caller
uint256 ethAmount = userProposalEth[_proposalId][msg.sender];
proposal.totalEth -= ethAmount;
userProposalEth[_proposalId][msg.sender] = 0;

However, withdrawContribution only zeroes the user's values, and not the totals. It's possible then to artificially increase a proposal balance, without truly depositing that quantity of ETH/tokens. This may lead to malicious proposal passing and/or stealing of assets from the migration contracts.

Proof of Concept

Alice starts a proposal.

1. She joins it with 1 ETH;
   • proposal.totalEth = 1 ETH
   • userProposalEth = 1 ETH
2. She withdrawContribution, getting 1 ETH back;
   • proposal.totalEth = 1 ETH
   • userProposalEth = 0 ETH
3. repeat 1-2;

With this method it's possible to increase the proposal's balance without spending anything. Alice can stop until her proposal reaches the true address(this).balance, so she can call commit stealing ETH from other proposals.

Same argument works for fractional tokens instead of ETH.

Recommended Mitigation Steps

Remove withdrawContribution, since it serves the same purpose as leave.

Lines of code

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

Vulnerability details

Impact

When committing a migration proposal, it's status is changed to isCommited = true and the buyout starts.

After 4 days, the buyout ends. Considering the case it was rejected, its status goes to INACTIVE. Nothing happens to the proposal in the Migration contract.

When someone makes another buyout (it doesn't matter if it starts from a migration), and it ends successfully, our rejected proposal can be suddenly settled. This is because in Migration#settleVault the requirements are only for the buyout to be SUCCESS and the proposal to be isCommited.

Proof of Concept

Someone starts a malicious proposal. It's easily commited by using a low targetPrice, but of course it's rejected during the Buyout. Later on, a valid Buyout is successful but immediately the exploiter can call settleVault and settleFractions to pass his proposal and steal from the vault.

Recommended Mitigation Steps

I believe it's hard to solve this without changing a little the architecture of the contracts. Maybe Migration.sol should have its own version of Buyout that tracks proposalId?

Lines of code

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

Vulnerability details

Impact

Everyone can start a Buyout for a vault by paying only 1 wei. For the next 4 days no other Buyout can start. If someone is fast enough, they can start another griefing buyout as soon as one finishes, meaning that it's possible to block the functionality of the contract forever.

Among others, these are functionality impacted:

1. it blocks serious buyouts (obv.);
2. it blocks redeem, meaning that even if someone has 100% the fractional tokens of a vault, they will never get the NFT back;
3. it blocks any migration, since the buyout needs to be inactive for every step;
4. all funds in Migration.sol can't be withdrawn, since leave wants the buyout inactive;

Blocking buyouts and migrations will make fractional tokens worthless, since there's no way to get the underlying back.

Proof of Concept

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

Recommended Mitigation Steps

Consider allowing to overwrite a buyout with another with better price. Alternatively, allow multiple buyouts active at the same time using an index, like with migration's proposals.

Also add a require(depositAmount > 0) in Buyout.start, so someone who has 100% of tokens can redeem immediately without a griefer making him wait.

(L1) Starting/joining/leaving a migration proposal shouldn't depend on Buyout state

In Migration.sol propose function (and similarly in join and leave):

// Reverts if buyout state is not inactive
(, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
State required = State.INACTIVE;
if (current != required) revert IBuyout.InvalidState(required, current);

I don't see the need to check the Buyout state at these steps. This check only slows a possible proposal initiation.

(L2) call instead of transfer

At the end of Migration.sol#leave and #withdrawContribution, ether is sent back to the caller.

// Withdraws ether from contract back to caller
payable(msg.sender).transfer(ethAmount);

As usual, if msg.sender is a contract that performs some operation on fallback, the transfer may out-of-gas and revert, leading to the impossibility to withdraw that funds. Consider using the low-level call instead. No re-entrancy issues, since this line is at the end of the function.

(L3) Migration proposals lasts indefinetely

According to the documentation, "For 7 days, users can contribute their fractions / ether to signal support" to a proposal (https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L23). However an user can call join for a proposal regardless of the time.

Consider adding a requirement in Migration.sol#join like require(block.timestamp <= proposal.startTime + PROPOSAL_PERIOD).

(N1) Emit an event on proposal creation

Something like a proposal creation needs to be easily caught off-chain. Consider adding a specific event at the end of Migration.sol#propose.

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

(N2) Events SellFractions and BuyFractions may need a vault parameter

Right now the events SellFractions and BuyFractions don't track for which vault the user selled their fractions. Such a parameter should be essential to track the buyout of a vault.

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

VaultRegistry.totalSupply(vault) to token.totalSupply(id)

When the total supply of fractional tokens of a vault is needed, we can make a call VaultRegistry.totalSupply(vault), that gets the token/id and then calls FERC1155(info.token).totalSupply(info.id).

However, sometimes we already have these values, so with this method we have a staticall and two warm SLOAD extra. In the following lines, consider using directly FERC1155(token).totalSupply(id).

Buyout.sol: L71, L210, L267, L288 Migration.sol: L95, L200, L470

VaultInfo.id can be uint96

token and id can be packed in a single storage slot if instead of using uint256 we use uint96. The number of vaults won't realistically reach 2**96.

for-loop i++

In general, a for-loop like this can be made more gas efficient.

for (uint256 i = 0; i < length; i++) {
    ...
}

Consider rewriting it like this:

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Lines

grep -r 'i++'

Migration.sol buyout can be immutable

SInce this variable doesn't change, it can be set immutable. It saves a SLOAD in every function.

nonReentrant modifier in join isn't needed

There's a nonReentrant modifier in Migration.sol#join. I don't a way to reenter from this function, so I suggest to remove it (to save gas of course).