Fractional v2 contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The user can lose ether due to incorrect proposalId in the method join of Migration contract.

Proof of Concept

In the join method of Migration contract, If the registry (migrationInfo[_vault][_proposalId]) does not exist, it will take a default structure where it will deposit the ether, but it can never be recovered since the proposalId is shared between the different vaults, so it does not have to match even if a new proposal is created.

Imagine the following scenario:

• User call join(_vault, 123, 0) with 1 ether
• The proposal 123 will be add this ether

        Proposal storage proposal = migrationInfo[_vault][_proposalId];
        // Updates ether balances of the proposal and caller
        proposal.totalEth += msg.value;
        userProposalEth[_proposalId][msg.sender] += msg.value;

• But the proposal 123 doesn't exists in this vault, so the user lose the ether.

Recommended Mitigation Steps

• Ensure that the proposal exists.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L38-L41 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L15-L17

Vulnerability details

Impact

The execute proof is bypassed by using the fallback method which allows third parties to perform transfer actions on accounts that are not in the merkletree.

Proof of Concept

Like you can see in the fallback method:

fallback(bytes calldata _data) external payable returns (bytes memory response) {
    address plugin = methods[msg.sig];
    (,response) = _execute(plugin, _data);
}

it will call _execute with an arbitrary data without any proof.

The _execute method contains the following protection to prevent a takeover:

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

However, if you change the nonce to 0, you could call the init method to takeover the contract, or just by changing the merkleRoot, you could already take a big advantage of the vault. You could also modify the owner, install a plugin, and restore the owner to the previous one, there are many possible scenarios like just drain the contract.

Recommended Mitigation Steps

• Redesing the fallback method to call execute.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L74 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L63 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L131

Vulnerability details

Impact

A malicious user can perform arbitrary burn and mint actions on the VaultRegistry.fNFT token. This allows an attacker to alter user balances for the VaultRegistry.fNFT token, handled by all vaults created with create and createFor methods of VaultRegistry.

Proof of Concept

Consider the following exploit for our proof of concept (it will run in delegatecall mode in our vault):

interface IRegistry
{
    function mint(address _to, uint256 _value) external;
    function burn(address _from, uint256 _value) external;
}
contract exploitContract
{
    address constant _attacker = address(0x0A..); // Attacker address
    address constant _registryAddress = address(0x0B..); // VaultRegistry.address
    uint256 constant _value = 123;

    function exploit() public
    {
        IRegistry(_registryAddress).mint(_attacker, _value);
        // IRegistry(_registryAddress).burn(_attacker, _value)
    }
}

Recipe:

• The attacker calls the createFor method of VaultRegistry with the following arguments:
  • bytes32 _merkleRoot = bytes32(0).
  • address _owner = _attacker.
  • address[] memory _plugins = new address[](0).
  • bytes4[] memory _selectors = new bytes4[](0).
• The owner of the vault is attacker because the IVault(vault).transferOwnership(_owner) and the token is address(fNFT).
• The attacker call to vault.execute with the following arguments:
  • address _target = our exploitContract.
  • bytes calldata _data = abi.encodeWithSignature('exploit()');
  • bytes32[] calldata _proof = new bytes32[](0)
    • Force an invalid merkle proof verification.

What happens is the following:

• The merkleRoot verification fails but since the msg.sender is the owner it doesn't matter (this is not relevant because having a correct merkle tree could be also done).

if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
+    if (msg.sender != owner)
       revert NotAuthorized(msg.sender, _target, selector);
}
 (success, response) = _execute(_target, _data);

• It calls _execute(_target, _data);
  • Where _target is our exploitContract and the _data is the call to our exploit() method.
• Our vault calls (success, response) = exploitContract.delegatecall{gas: stipend}(_data);
• So our vault executes the logic of our exploitContract contract in the vault context.
• Which calls _registryAddress to do either mint or burn.

    function burn(address _from, uint256 _value) external {
        VaultInfo memory info = vaultToToken[msg.sender];
        uint256 id = info.id;
        if (id == 0) revert UnregisteredVault(msg.sender);
+       FERC1155(info.token).burn(_from, id, _value);
    }

• Since the msg.sender is our vault, the info.token is address(fNFT) so the burn being called from registry will bypass the onlyRegistry modifier, resulting in a loss of funds.

It's also possible to do it with create but it require to use plugins and selectors.

Recommended Mitigation Steps

• Prevent multiple vaults from having access to the same token.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L41-L42

Vulnerability details

Impact

The user's ether could be locked and unrecoverable.

Proof of Concept

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected lines:

• Migration.sol#L172
• Migration.sol#L325

Recommended Mitigation Steps

• Use .call instead of .transfer

• Low
  • 1. Packages with vulnerabilities
  • 2. Outdated compiler
  • 3. Unsafe ERC20 calls
  • 4. Lack of ACK during owner change
  • 5. Lack of nonReentrant in Buyout and Migration
  • 6. Lock ether
  • 7. Complex fallback
• Non-Critical
  • 8. Outdated packages
  • 9. Lack of checks
  • 10. Open TODO
  • 11. Use abstract for base contracts
  • 12. install allows uninstall
  • 13. Contracts with functions without auth that can produce economic losses
  • 14. Improve propose method design

Low

1. Packages with vulnerabilities

The project contains packages that urgently need to be updated because they contain important vulnerabilities.

npm audit:

54 vulnerabilities (11 moderate, 40 high, 3 critical)

2. Outdated compiler

The pragma version used is:

pragma solidity 0.8.13;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

3. Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• EIP-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above). Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for transfer:

function ERC20Transfer(
    address _token,
    address _to,
    uint256 _value
) external {
-   IERC20(_token).transfer(_to, _value);
+   require(IERC20(_token).transfer(_to, _value));
}

• TransferReference.sol#L22
• SafeSend.sol#L33

Affected source code for transferFrom:

• BaseVault.sol#L65

4. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• Vault.sol#L93
• FERC1155.sol#L234

5. Lack of nonReentrant in Buyout and Migration

ERC1155 tokens incorporate the ability to react to a transfer using the onERC1155Received method on the receiver. And in the Buyout contract there is an alteration of the states after the call to safeTransferFrom, so a reentry is possible.

IERC1155(token).safeTransferFrom(
    address(this),
    msg.sender,
    id,
    _amount,
    ""
);
// Updates ether balance of pool
buyoutInfo[_vault].ethBalance += msg.value;

In the end method, you play with fire by sending ether after the transfer, saving the reentrancy by deleting the buyoutInfo[_vault];

Safe Transfer Rules To be more explicit about how the standard safeTransferFrom and safeBatchTransferFrom functions MUST operate with respect to the ERC1155TokenReceiver hook functions, a list of scenarios and rules follows.

The definition of the burn method of an ERC1155 is as follows:

A contract MAY skip calling the ERC1155TokenReceiver hook function(s) if the mint operation is transferring the token(s) to itself. In all other cases the ERC1155TokenReceiver rules MUST be followed as appropriate for the implementation (i.e. safe, custom and/or hybrid).

Reference:

• https://eips.ethereum.org/EIPS/eip-1155#erc-1155-token-receiver

So some implementations that follow the standard of an ERC1155 could also trigger a reentrancy.

In the case of the settleFractions method of the Migration contract, reentrancy is possible since the fractionsMigrated flag is set after minting, which would allow the contract to be minted multiple times, however the receiver of the ERC1155TokenReceiver event is address(this) and is considered non-exploitable.

Under all these premises, I consider that the nonReentrant modifier is needed in the following methods:

• Buyout.sol#L168-L176
• Buyout.sol#L226-L235
• Buyout.sol#L221
• Buyout.sol#L264-L270
• Migration.sol#L164-L172
• Migration.sol#L279
• Migration.sol#L312-L325

6. Lock ether

If someone sends ether to the Buyout contract without using the buy/sell methods, via the receive method, they will be blocked forever. A similar behavior occurs in the Migration contract.

Affected source code:

• Buyout.sol#L53
• Migration.sol#L63

7. Complex fallback

The fallback method iss too complex and can be denied in certain cases.

According to the fallback solidity (documentation](https://docs.soliditylang.org/en/develop/contracts.html#fallback-function)

In the worst case, if a payable fallback function is also used in place of a receive function, it can only rely on 2300 gas being available (see receive Ether function for a brief description of the implications of this).

In the worst case, the receive function can only rely on 2300 gas being available (for example when send or transfer is used), leaving little room to perform other operations except basic logging. The following operations will consume more gas than the 2300 gas stipend:

• Writing to storage
• Creating a contract
• Calling an external function which consumes a large amount of gas
• Sending Ether

Reference:

• https://docs.soliditylang.org/en/develop/contracts.html#fallback-function

Affected source code:

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

---

Non-Critical

8. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "^4.6.0"

last 4.7.0

9. Lack of checks

Check for address(0) during constructor, otherwise it could lead to bad initialization, bad implementations, or bad admin changes.

Affected source code for address(0):

• Supply.sol#L17
• Metadata.sol#L17
• Minter.sol#L18
• BaseVault.sol#L25
• Buyout.sol#L47-L49
• Migration.sol#L58-L59

Royalty _percentage must be less than 100, otherwise it could result in a Denial of Service in royaltyInfo:

• FERC1155.sol#L223

10. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

• MerkleBase.sol#L24

11. Use abstract for base contracts

Abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• Minter.sol#L12

12. install allows uninstall

It is possible to use the method install for uninstall, and the emmited event will be different. Being able to affect the correct functioning of the dApps that consume it. You only need to specify an address(0) as plugin during install.

Affected source code:

• Vault.sol#L73

13. Contracts with functions without auth that can produce economic losses

The BaseVault, Transfer or TransferReference contracts allows an attacker to steal all the tokens to the implementation it has without having any type of auth, although it is not expected to have tokens, and trying to call with a delegate call is a risk that must be known.

function ERC20Transfer(
    address _token,
    address _to,
    uint256 _value
) external {
   IERC20(_token).transfer(_to, _value);
}

Affected source code:

• Transfer.sol#L13
• TransferReference.sol#L17-L69
• BaseVault.sol#L53-L117

The same thing happens in the Supply contract with mint and burn:

/// @notice Mints fractional tokens
/// @param _to Target address
/// @param _value Transfer amount
function mint(address _to, uint256 _value) external {
    IVaultRegistry(registry).mint(_to, _value);
}

/// @notice Burns fractional tokens
/// @param _from Source address
/// @param _value Burn amount
function burn(address _from, uint256 _value) external {
    IVaultRegistry(registry).burn(_from, _value);
}

Affected source code:

• Supply.sol#L11
• SupplyReference.sol#L22-L31

14. Improve propose method design

The propose method of Migration contract should return the proposalId in order to avoid human errors like the one mentioned in the "User may lose ether due to incorrect proposalId.

Affected source code:

• Migration.sol#L72

• Gas
  • 1. constants expressions are expressions, not constants
  • 2. Reorder structure layout
  • 3. Use calldata instead of memory
  • 4. There's no need to set default values for variables
  • 5. Don't use the length of an array for loops condition
  • 6. ++i costs less gas compared to i++ or i += 1
  • 7. Reduce the size of error messages Long revert Strings
    • Use Custom Errors instead of Revert Strings to save Gas
  • 8. Use library instead of abstract contract
  • 9. Use inline methods
  • 10. Gas saving using immutable
  • 11. Improved migrateFractions logic

---

Gas

1. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Affected source code:

• Permit.sol#L5-L17

2. Reorder structure layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

Put booleans types together on IMigration.sol#L8-L33

struct Proposal {
    // Start time of the migration proposal
    uint256 startTime;
    // Target buyout price for the migration
    uint256 targetPrice;
    // Total ether contributed to the migration
    uint256 totalEth;
    // Total fractions contributed to the migration
    uint256 totalFractions;
    // Module contract addresses proposed for the migration
    address[] modules;
    // Plugin contract addresses proposed for the migration
    address[] plugins;
    // Function selectors for the proposed plugins
    bytes4[] selectors;
+   // Old fraction supply for a given vault
+   uint256 oldFractionSupply;
+   // New fraction supply for a given vault that has succesfully migrated
+   uint256 newFractionSupply;
    // Address for the new vault to migrate to (if buyout is succesful)
    address newVault;
    // Boolean status to check if the propoal is active
    bool isCommited;
-   // Old fraction supply for a given vault
-   uint256 oldFractionSupply;
-   // New fraction supply for a given vault that has succesfully migrated
-   uint256 newFractionSupply;
    // Boolean status to check that the fractions have already been migrated
    bool fractionsMigrated;
}

3. Use calldata instead of memory

The following methods are external but memory is used for arguments.

• IMigration.sol#L89-L162
• IModule.sol#L8-L13
• IVault.sol#L43-L64
• IVaultRegistry.sol#L43-L71

These one require to change the visibility to external:

• MerkleBase.sol#L61
• MerkleBase.sol#L73

4. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• MerkleBase.sol#L51
• BaseVault.sol#L64
• BaseVault.sol#L83
• BaseVault.sol#L107
• Vault.sol#L78
• Vault.sol#L104

5. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• MerkleBase.sol#L51
• MerkleBase.sol#L110
• BaseVault.sol#L64
• BaseVault.sol#L83
• BaseVault.sol#L107
• BaseVault.sol#L130-L132
• Buyout.sol#L454

6. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• Vault.sol#L78
• Vault.sol#L104

7. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• MerkleBase.sol#L62
• MerkleBase.sol#L78

8. Use library instead of abstract contract

If the following contracts were libraries, they could save gas thanks to compiler optimizations since there are functions that are not called and being an abstract contract implies publishing all the code marked as public, such as library, the compiler can choose which functions to eliminate by not be used.

Smaller contracts would be produced, with less inheritance and therefore more auditable and readable.

Affected source code:

• MerkleBase.sol

9. Use inline methods

The following methods can be moved to inline calls without greatly affecting readability, this will increase the performance of the contract.

Affected source code:

• Multicall.sol#L39

10. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• BaseVault.sol#L19
• Minter.sol#L14
• VaultFactory.sol#L15
• Buyout.sol#L29-L33
• Migration.sol#L37-L39

11. Improved migrateFractions logic

migrateFractions method could be optimized as follows:

-       (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
+       (, address proposer, State current, , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
            _vault
        );
        State required = State.SUCCESS;
        if (current != required) revert IBuyout.InvalidState(required, current);
        // Reverts if proposer of buyout is not this contract
        if (proposer != address(this)) revert NotProposalBuyout();
-
-       // Gets the last total supply of fractions for the vault
-       (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
-           _vault
-       );

There is no need to duplicate the call of buyoutInfo(_vault)