Fractional v2 contest - scaraven's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L86-L88 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L267-L269

Vulnerability details

Impact

When a buyout starts, the calculation for fractionPrice and buyoutPrice has several arithmetic rounding problems. This causes the fractionPrice to be lower than it should be so users who sell fractional tokens into the pool receive less then they should. Therefore, if a buyout bid does pass, users who did not sell to the pool will receive more ethereum then those who did sell. This provides incentive against selling to the pool and can make it much harder for buyout bids to become successful.

        uint256 buyoutPrice = (msg.value * 100) /
            (100 - ((depositAmount * 100) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

Proof of Concept

1. Bob starts a buyout bid with 1e6 wei, depositAmount of 50 and totalSupply of 1001
2. The fractionPrice will be become 1040 while the actual value should be 1051
3. Every user who sells tokens will receive 1051-1040=11 less wei per token then they should
4. Say approx 50% of the tokens are sold so the pool now has 501 tokens and ethBalance = 1e6 - (501-50)*1040 = 530960
5. Alice who did not sell her tokens during the buyout period now sells her tokens, she receives 530960/(1001-501) = 1061 wei per token which is 1061-1040 = 21 more than bob got

Tools Used

VS Code

Recommended Mitigation Steps

Use a higher precision when calculating e.g. 1e6 instead of 100

        uint256 buyoutPrice = (msg.value * 1e6) /
            (100 - ((depositAmount * 1e6) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L73-L82 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L72-L99 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L174

Vulnerability details

Impact

In propose() in Migration.sol, there is no check that the lengths of the selectors and plugins arrays are the same. This means that if a migration is successful, the install() function in Vault.sol could revert beacuse we access an array out of bounds. This prevents a new vault being created thereby permanently locking assets inside the vault.

Proof of Concept

1. user starts a new migration proposal where selectors.length != plugins.length
2. enough users join proposal and the buyout bid starts
3. buyout bid is successful and migration starts with settleVault()
4. a new vault is cloned with create() -> registry.deployFor() -> vault.install(selectors, plugins)
5. a. If selectors.length > plugins.length then we get an out of bounds error and transaction reverts b. If selectors.length < plugins.length then the excess values in plugins is ignored which is tolerable
6. In scenario a., the migration fails and a new migration cannot start so assets in the vault are permanently locked

This may seem quite circumstantial as this problem only occurs if a user specifies selectors and plugins wrongly however it is very easy for an attacker to perform this maliciously with no cost on their behalf, it is highly unlikely that users will be able to spot a malicious migration.

Tools Used

VS Code

Recommended Mitigation Steps

Consider adding a check in propose() to make sure that the lengths match i.e.

function propose(
        address _vault,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        uint256 _newFractionSupply,
        uint256 _targetPrice
    ) external {
        // @Audit Make sure that selectors and plugins match
        require(_selectors.length == _plugins.length, "Plugin lengths do not match");
        // Reverts if address is not a registered vault
        (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
        if (id == 0) revert NotVault(_vault);
        // 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);

        // Initializes migration proposal info
        Proposal storage proposal = migrationInfo[_vault][++nextId];
        proposal.startTime = block.timestamp;
        proposal.targetPrice = _targetPrice;
        proposal.modules = _modules;
        proposal.plugins = _plugins;
        proposal.selectors = _selectors;
        proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
            _vault
        );
        proposal.newFractionSupply = _newFractionSupply;
    }

Additionally, I would suggest adding such a check in the install() function as this may prevent similiar problems if new modules are added

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/SafeSend.sol#L33 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325

Vulnerability details

Impact

As outlined in this report here, there are several scenarios where transfer() can fail and it is always better to use call() instead when sending eth

Tools Used

VS Code

Recommended Mitigation Steps

Use call() instead of transfer()

Lines of code

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

Vulnerability details

Impact

In the start() function in Buyout.sol, due to there being no minimum msg.value or depositAmount required for a user to start a Buyout bid, an attacker can constantly start buyout bids with no value thereby preventing anyone from starting an authentic buyout bid until the bid ends (after 4 days). This comes at negligible cost for the attacker and renders the Buyout contract completely useless.

Proof of Concept

1. An attacker calls start() with a msg.value of 1 wei and zero fractional tokens.

• As msg.value is non-zero, we pass the check here

2. buyoutPrice will be set to 1 wei and fractionPrice will become 0 due to arithmetic rounding
3. The buyout has no value for any fractional token holders as they receive 0 eth for selling tokens into the pool
4. Other users cannot start a Buyout bid because STATE current is now LIVE
5. In the very unlikely case that the buyout is successful, the attacker gains full control of all assets in the vault and spends only 1 wei
6. 99% of the time, the bid will be unsucessful and 4 days have been wasted on a pointless buyout bid
7. The attacker can repeat step 1 after the 4 day period

Tools Used

VS Code

Recommended Mitigation Steps

There are two possible fixes:

1. Add a minimum msg.value which is easy to implement but has several problems. If the value of the underying assets is too low then it forces people to overpay for a buyout and if the minimum is too low then an attacker can still grief the buyout process.
2. Add a system similiar to the propose() method used in Migration.sol where multiple buyouts can be started independently of each other and fractional token holders can contribute to any buyout they wish to. Therefore someone starting a useless bid does not affect the ability of a genuine buyout bid to start.

Summary

The developers have opted for a highly optimized and flexible smart contract architecture with the use of quite novel code (createWithImmutableArgs) and extensive assembly. However, as the developers are most likely fully aware, comes at the cost of much lower readability and can make the architecture and interdependence between different contracts quite confusing. I believe a small flow diagram to showcase how an example architecture would be deployed would be a massive help in understanding the architecture. On the other hand, the developers went above and beyond in commenting the code which was vitally important especially for such a complex architecture. In the future, I would be very careful when developing and adding new modules as in this case the contract is only as strong as its weakest link. When adding a new module, I would not only make sure that the module itself is secure but if it interacts with other modules then it does not introduce new vulnerabilities as the interdependence between modules is not always straightforward.

Issue #1 Incorrect memory expansion gas cost calculation

Code used to calculate memory expansion cost is inconsistent with equation 326 in the ethereum yellow paper

This can cause incorrect errors to be reported

Currently the code is cost += (returnDataWords-msizeWords) * COST_PER_WORD + (returnDataWords**2 - msizeWords**2) / MEMORY_EXPANSION_COEFFICIENT when it should be cost += (returnDataWords-msizeWords) * COST_PER_WORD + (returnDataWords - msizeWords)**2 / MEMORY_EXPANSION_COEFFICIENT

Change

                        cost := add(
                            cost,
                            add(
                                mul(
                                    sub(returnDataWords, msizeWords),
                                    COST_PER_WORD
                                ),
                                div(
                                    sub(
                                        mul(returnDataWords, returnDataWords),
                                        mul(msizeWords, msizeWords)
                                    ),
                                    MEMORY_EXPANSION_COEFFICIENT
                                )
                            )
                        )

to

                        cost := add(
                            cost,
                            add(
                                mul(
                                    sub(returnDataWords, msizeWords),
                                    COST_PER_WORD
                                ),
                                div(
                                    mul(
                                        sub(returnDataWords, msizeWords),
                                        sub(returnDataWords, msizeWords)
                                    ),
                                    MEMORY_EXPANSION_COEFFICIENT
                                )
                            )
                        )

Occurences: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Supply.sol#L66-L81 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Supply.sol#L156-L171 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Transfer.sol#L96-L117 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Transfer.sol#L281-L296 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Transfer.sol#L412-L427 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/targets/Transfer.sol#L642-L657

Issue #2 Storage check in _execute() from Vault.sol is obselete

Currently _execute() contains the line:

        if (owner_ != owner) revert OwnerChanged(owner_, owner);

to make sure that the owner storage variable is not modified after the delegatecall. This check can be easily bypassed by modifying the other storage variables such as nonce (allows a user to reinitalise contract), merkleRoot (user can execute malicious permissions) or methods (a malicious plugin can be installed)

1. Either you can trust that the _target contracts will all be stateless and just remove the check to save gas OR
2. add extra checks to include the other storage variables (recommended)

Occurences: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L132

Issue #3 Use of magic values

Throughout the contracts, there is an inconsistent use of magic values where sometimes constants are used and sometimes magic values are used

e.g. Change

        uint256 buyoutPrice = (msg.value * 100) /
            (100 - ((depositAmount * 100) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

to

uint256 public immutable PRECISION = 100;
...
        uint256 buyoutPrice = (msg.value * PRECISION) /
            (PRECISION - ((depositAmount * PRECISION) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

Occurences: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L86-L88 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L208-L211 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L451 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L199 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Minter.sol#L37 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L315 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L452

Issue #4 No check that newVault is not zero address in migrateFractions()

In migrateFractions() from Migration.sol, the newVault address is read with no check that the address is not the zero address (which can occur if someone calls migrateFractions() before settleVault() is called). This causes the transaction to revert with no proper error message when the function attempts to send tokens.

Consider adding a check to produce a custom error if newVault == address(0)

Occurences: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L466