John_Femi - Griefing Attack on TOFT and mTOFT contract deployment

[sherlock-admin2]

John_Femi

medium

Griefing Attack on TOFT and mTOFT contract deployment

Summary

The TOFTVault is deployed and ownership transferred to zero address, then the mTOFT or TOFT is expected to claim ownership of the vault on deployment, but the delay between the deployment of the vault and claiming of ownership of vault can be exploited and an attacker claim ownership by frontrunning the TOFT contract deployment, causing the deployment to revert but expensive gas fee still paid.

Vulnerability Detail

In the constructor of the TOFTVault the ownership of the vault is transferred to the address 0 as seen at https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/TOFTVault.sol#L43

The claimOwnership function has no access control and anyone can call the function as seen at https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/TOFTVault.sol#L73

It is expected that TOFT claims the vault ownership here at https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/TOFT.sol#L83

But the time difference between the two contract deployments opens up the possibility of an attacker monitoring the mempool to call the claimOwnership function ahead of the TOFT contract deployment fulfillment. Though this causes no loss of funds, the contract deployment fails and the expensive deployment gas fee is still spent, causing grief to the team on the deployment of the TOFT or mTOFT contract. Deploying another TOFTVault and repeating the same process yields the same result

Impact

Medium Impact as it causes no huge loss of funds except the ones spent on gas but can cause grief on the project by delaying the launch and deployment of the TOFT smart contract

Code Snippet

 /// @dev Intended to be called once by the TOFT contract
    function claimOwnership() external {
        if (owner() != address(0)) revert OwnerSet();
        _transferOwnership(msg.sender);
    }

    constructor(TOFTInitStruct memory _tOFTData, TOFTModulesInitStruct memory _modulesData)
        BaseTOFT(_tOFTData)
        ERC20Permit(_tOFTData.name)
    {
        // Set TOFT execution modules
        if (_modulesData.tOFTSenderModule == address(0)) revert TOFT_NotValid();
        if (_modulesData.tOFTReceiverModule == address(0)) {
            revert TOFT_NotValid();
        }
        if (_modulesData.marketReceiverModule == address(0)) {
            revert TOFT_NotValid();
        }
        if (_modulesData.optionsReceiverModule == address(0)) {
            revert TOFT_NotValid();
        }
        if (_modulesData.genericReceiverModule == address(0)) {
            revert TOFT_NotValid();
        }

        _setModule(uint8(ITOFT.Module.TOFTSender), _modulesData.tOFTSenderModule);
        _setModule(uint8(ITOFT.Module.TOFTReceiver), _modulesData.tOFTReceiverModule);
        _setModule(uint8(ITOFT.Module.TOFTMarketReceiver), _modulesData.marketReceiverModule);
        _setModule(uint8(ITOFT.Module.TOFTOptionsReceiver), _modulesData.optionsReceiverModule);
        _setModule(uint8(ITOFT.Module.TOFTGenericReceiver), _modulesData.genericReceiverModule);

        vault = IToftVault(_tOFTData.vault);
        vault.claimOwnership();

        if (address(vault._token()) != erc20) revert TOFT_VaultWrongERC20();
    }

Tool used

Manual Review, Forge

Recommendation

Instead of claiming ownership on contract deployment, deploy the TOFT contract and have an admin/onlyOwner function transfer ownership to the newly deployed TOFT contract with _transferOwnership. This is done once and ownership of the vault is successfully transferred.

[maarcweiss]

Invalid/Low this is the same of front-running the initializing on proxies that has been standardized as low.

[nevillehuang]

Invalid based on similar impacts of the following sherlock rule. A redeployment can be initiated

7. Front-running initializers: Front-running initializers where there is no irreversible damage or loss of funds & the protocol could just redeploy and initialize again is not a valid issue.