Fractional v2 contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The migrateFractions() function can be called multiple times by the same user allowing them to withdraw more fractions than they should be able to.

Proof of Concept

Here's a test as the PoC:

// Migration.t.sol

    function testMigrationMultipleTimes() public {
        testSettle();
        (, , , , address newVault, , , , ) = migrationModule.migrationInfo(
            vault,
            1
        );
        (address newToken, uint256 id) = registry.vaultToToken(newVault);

        assertEq(getFractionBalance(alice.addr), 4000);
        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 6000);

        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 12000);

        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 18000);
    }

Tools Used

none

Recommended Mitigation Steps

add mapping(uint => address) claimed where the key is the proposalID and the value is the user's address. Then, check whether the user has already claimed in the migrateFractions() function.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L223

Vulnerability details

Impact

Generally, when accessing the contract's royalties, the caller will use the following schema:

uint amount = x;
(address receiver, uint royalty) = c.royaltyInfo(id, amount);
token.transfer(receiver, royalty);
token.transfer(owner, amount - royalty); // send remaining balance to the owner of the token.

If the royalty is larger than 100%, the last line will trigger an underflow.

Proof of Concept

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L223

Allows the controller to set whatever value they want.

Tools Used

none

Recommended Mitigation Steps

Use a reasonable limit, e.g. 10%

Lines of code

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

Vulnerability details

Impact

You should use call() instead of transfer(). If the recipient is a contract it might use more than 2300 gas which will make the transaction fail if the ETH was sent with transfer().

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

Tools Used

none

Recommended Mitigation Steps

Use call() instead.

Lines of code

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

Vulnerability details

Impact

As described in the module's docs, a vault can only have a single active Buyout proposal at any time. An attacker is able to DOS the module by repeatedly creating a proposal with a "weird" configuration. That would make the module unusable for honest participants.

Proof of Concept

The easiest "weird" configuration looks like this: The attacker doesn't have any fractional tokens. They create a proposal with 1 wei balance. Users are now able to sell their fractional for 1 wei. That's it. Since no one will probably do that, you effectively created an useless proposal:

// Buyout.t.sol

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

        assertEq(getFractionBalance(bob.addr), 0);
        assertEq(getFractionBalance(buyout), 0);
        assertEq(getETHBalance(bob.addr), INITIAL_BALANCE);
        assertEq(getETHBalance(buyout), 0 ether);

        bob.buyoutModule.start{value: 1}(vault);

        assertEq(getFractionBalance(bob.addr), 0);
        assertEq(getFractionBalance(buyout), 0);
        assertEq(getETHBalance(buyout), 1);
    }

This is possible because the ERC1155 contract you use doesn't block transfers of 0 tokens.

Even if it didn't you could still DOS the module with the cost of a single fractional token. If the attacker has enough ETH they can also create a proposal with an absurdly high fractionPrice. Blocking anybody from buying or selling tokens.

Tools Used

none

Recommended Mitigation Steps

The issue is the limit of one buyout at a time. I'm not entirely sure, but would allowing multiple concurrent proposals break anything? As long as one of them reaches the limit of 51% you have the same outcome.

Report

• Low
  • L-01: Buyout phases don't match with the documentation
  • L-02: Buyout auction success threshhold doesn't match the one in the documentation
• Non-Critical
  • N-01: emit an event when changing a contract's configuration

Low

L-01: Buyout phases don't match with the documentation

According to the contract's comment, there are two phases for an auction, the proposal & rejection period. The former lasts 2 days and the other one lasts 4 days. Reading that, one would expect them to be two distinct phases. So in the first two days you're only able to sell your fractional tokens. Then, the next 4 days you're only allowed to buy fractional tokens. After that, the auction is finished (full duration is 6 days). Instead, you can sell only the first two days while you can buy from the beginning to the end (4 days).

• https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L19
• https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L162
• https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L125
• https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L203

L-02: Buyout auction success threshold doesn't match the one in the documentation

According to the contract's comment, the pool needs more than 51% of the total supply for it to succeed. But, the actual implementation expects only more than 50%. Either update the documentation or the implementation.

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

Non-Critical

N-01: emit an event when changing a contract's configuration

They should be accompanied by an event to allow third parties to easily track them.

• https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L199

Report

G-01: use calldata instead of memory for array parameters

The gas for calldata parameters is already paid. It'll be cheaper.

e.g.

• 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#L101

G-02: use unchecked when incrementing the iterator of a for loop

Since i can't overflow because of the boundary set in the for loop, you can use unchecked to increment it. That will save a little gas

e.g.

• https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L104
• https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L78