Improper Validation Of create2 Return Value

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L362-L376
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/TapiocaDeployer/TapiocaDeployer.sol#L22-L50
https://github.com/boringcrypto/BoringSolidity/blob/master/contracts/BoringFactory.sol#L32-L68

Vulnerability details

Impact

There is no validation for deployed contracts using BoringFactory whether the returned address is non-zero address and check for the size of code. This will result in setting zero address and nonexisting address as isMarketRegistered equal to true.

Proof of Concept

Inside Penrose.sol#registerSingularity() it uses deploy() function of BoringFactory.deploy() function of BoringFactory does not revert properly if there is a failed contract deployment or revert from the create2 opcode as it does not properly check the returned address for bytecode and for non-zero address. The create2 opcode returns the expected address which will never be the zero address. I have created an issue on BoringSolidity library about this but never received any response till now.

The same issue was found by leastwood on the mochi protocol. In their case BeaconProxyDeployer library was vulnerable and in our case BoringSolidity's BoringFactory library is vulnerable.
issue by leastwood : code-423n4/2021-10-mochi-findings#155

I saw another place in the contract where while deploying the contract using create2 it checks for zero address but didn't check for size of code

here : https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/TapiocaDeployer/TapiocaDeployer.sol#L22-L50

Tools Used

Manual code review
Discussions with the LZ team

Recommended Mitigation Steps

Check whether the returned address is non-zero address and size of address

Fix for Penrose.sol#registerSingularity()

    function registerSingularity(
        address mc,
        bytes calldata data,
        bool useCreate2
    )
        external
        payable
        onlyOwner
        registeredSingularityMasterContract(mc)
        returns (address _contract)
    {
        _contract = deploy(mc, data, useCreate2);
        require(extcodesize(_contract) != 0, "returned contract don't have size");
        require(_contract != address(0), "zero address");
        isMarketRegistered[_contract] = true;
        emit RegisterSingularity(_contract, mc);
    }

Fix for TapiocaDeployer.sol#deploy()

function deploy(
        uint256 amount,
        bytes32 salt,
        bytes memory bytecode,
        string memory contractName
    ) external payable returns (address addr) {
        require(
            address(this).balance >= amount,
            "Create2: insufficient balance"
        );
        require(
            bytecode.length != 0,
            string.concat(
                "Create2: bytecode length is zero for contract ",
                contractName
            )
        );
        /// @solidity memory-safe-assembly
        assembly {
            addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt)
        }
        require(extcodesize(addr) != 0, "contract didn't have size");
        require(
            addr != address(0),
            string.concat(
                "Create2: Failed on deploy for contract ",
                contractName
            )
        );
    }

Assessed type

Library

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

0xRektora marked the issue as disagree with severity

[0xRektora]

Informational. Penrose can unregister a SGL if it is invalid. TapiocaDeployer is used only on Protocol genesis. No harm done.

[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