TOFT creation via `TapiocaWrapper::createTOFT` and `TapiocaOFT`/`mTapiocaOFT` implementations in `tapicaz` project are implemented incorrect in conjunction to each other

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/TapiocaWrapper.sol#L191 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/TapiocaWrapper.sol#L209 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/TapiocaOFT.sol#L21 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/mTapiocaOFT.sol#L9

Vulnerability details

Impact

Project tapiocaz contains logic to deploy and create Wrappers Tapoica OFTs. TapiocaWrapper is used to create TOFTs and TapiocaOFT/mTapiocaOFT are given bases to be used when deploying an TOFT via TapiocaWrapper::createTOFT. There are 3 major issues with these:

• TapiocaWrapper::createTOFT attempts to checks incorrectly that the provided bytecode is compatible with ITapiocaOFT,
• neither TapiocaOFT nor mTapiocaOFT extend the interface and by luck the incorrect check is passed
• TapiocaWrapper::createTOFT has an extra flag as input that should enforce that the provided bytecode is mTapiocaOFT compatible (has multichain wrapping/unwrapping support) but it does nothing (incorrectly implemented)

consequently:

• deployed contracts via createTOFT may not be ITapiocaOFT, an essential project requirement.

• any deployed contract with the intent of having multichain wrapping/unwrapping support may actually not be. This is of an high enough importance to protocol logic as to warrant a check in itself, otherwise the incorrect check (just a contract cast) would not exist.

• also, the incomplete TapiocaOFT and mTapiocaOFT implementations (missing ITapiocaOFT::harvestFees code) results a revert when TapiocaWrapper::harvestFees is called if using these provided implementations. Project documentation itself indicates to use the TapiocaWrapper and TapiocaOFT together as such the missing code is not intentional.

Deploy the TapiocaWrapper contract on the host chain of the ERC20, and the chain where you want it to be present too.

Deploy the TapiocaOFT contract on the host chain .

Proof of Concept

A detailed issue explication follows:

TapiocaWrapper::createTOFT checks, incorrectly, that the deployed contract (via TapiocaWrapper::_createTOFT) is ITapiocaOFT by comparing the passed ERC20 contract address with tha contract state saved one:

    if (address(iOFT.erc20()) != _erc20) {
        revert TapiocaWrapper__FailedDeploy();
    }

The intent to create an ITapiocaOFT compliant token is evident by the casting

    ITapiocaOFT iOFT = ITapiocaOFT(
        _createTOFT(_erc20, _bytecode, _salt, _linked)
    );

and the storing in variables that use the ITapiocaOFT type: tapiocaOFTs, tapiocaOFTsByErc20 and harvestableTapiocaOFTs

    /// @notice Array of deployed TOFT contracts.
    ITapiocaOFT[] public tapiocaOFTs;
    /// @notice Array of harvestable TOFT fees.
    ITapiocaOFT[] private harvestableTapiocaOFTs;
    /// @notice Map of deployed TOFT contracts by ERC20.
    mapping(address => ITapiocaOFT) public tapiocaOFTsByErc20;

and also usage of specific ITapiocaOFT function harvestFees:

    /// @notice Harvest fees from all the deployed TOFT contracts. Fees are transferred to the owner.
    function harvestFees() external {
        for (uint256 i = 0; i < harvestableTapiocaOFTs.length; i++) {
            harvestableTapiocaOFTs[i].harvestFees();
        }
        emit HarvestFees(msg.sender);
    }

Next, the intent to use TapiocaWrapper and TapiocaOFT together is stated in the project documentation

Deploy the TapiocaWrapper contract on the host chain of the ERC20, and the chain where you want it to be present too.

Deploy the TapiocaOFT contract on the host chain .

and in code, the intent to use TapiocaOFT (and mTapiocaOFT) as an ITapiocaOFT compliant token (and assumption) is evident by the castings to TapiocaOFT, to mTapiocaOFT (and the name basically):

        if (!_linked) {
            TapiocaOFT toft = TapiocaOFT(
                payable(
                    Create2.deploy(
                        // ...
                    )
                )
            );
            oft = address(toft);
        } else {
            mTapiocaOFT toft = mTapiocaOFT(
                payable(
                    Create2.deploy(
                        // ...
                    )
                )
            );
            oft = address(toft);
        }

While this casting is redundant, it also shows that the _linked, which is passed from TapiocaWrapper::createTOFT also has no utility.

The incorrect assumption here is that, casting an address is Solidity automatically checks interface support. This is false, and the currently used standard way of achieving this is to implemented ERC-165.

The intended check by the _linked variable would of been to be sure that the deployed bytecode is also mTapiocaOFT compliant.

Coming back to TapiocaOFT and mTapiocaOFT; these implementation do not extend the ITapiocaOFT but by project design do have most of the interface functions and in particular those that are called, from that interface, in the TapiocaWrapper::createTOFT function: erc20() and hostChainID().

Contracts inheritance view outlying the lack ITapiocaOFT extension and interface functions existing:

• TapiocaOFT

  • BaseTOFT and ERC20Permit
    • BaseTOFTStorage
      • OFTV2 (LayerZero libcode from here down the inheritance line)

• mTapiocaOFT

  • BaseTOFT and ERC20Permit
    • BaseTOFTStorage
      • OFTV2 (LayerZero libcode from here down the inheritance line)

Tools Used

Manual analysis

Recommended Mitigation Steps

Implement ERC-165 for ITapiocaOFT, TapiocaOFT and mTapiocaOFT and check them accordingly in each step of the TOFT creation in TapiocaWrapper; mTapiocaOFT check to be done only when the _link is set.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

0xRektora marked the issue as disagree with severity

[0xRektora]

informational severity, admin func called only by protocol. Probability are very low for this to happen, protocol can quickly find out if the deployed bytecode is invalid without damaging itself.

[c4-sponsor]

0xRektora marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b