Taiko - zzebra83's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/L1/hooks/AssignmentHook.sol#L113-L116 https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/L1/libs/LibProposing.sol#L85-L87 https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/L1/libs/LibProposing.sol#L249-L255

Vulnerability details

Impact

Proposal of new blocks triggers a call to proposeBlock in the libProposing library. In that function, there is this the following block of code:

        if (params.coinbase == address(0)) {
        params.coinbase = msg.sender;
    }

This sets the params.coinbase variable set by the caller of the function to be the msg.sender if it was empty.

As part of the process of proposal, hooks can be called of type AssignmentHook. An assignment hook's onBlockProposed will be triggered as follows:

                // When a hook is called, all ether in this contract will be send to the hook.
            // If the ether sent to the hook is not used entirely, the hook shall send the Ether
            // back to this contract for the next hook to use.
            // Proposers shall choose use extra hooks wisely.
            IHook(params.hookCalls[i].hook).onBlockProposed{ value: address(this).balance }(
                blk, meta_, params.hookCalls[i].data
            );

Notice how the meta data is passed to this function. Part of the function of the onBlockProposed is to pay the assigned prover their fee and the payee should be the current proposer of the block. this is done as follows:

        // The proposer irrevocably pays a fee to the assigned prover, either in
    // Ether or ERC20 tokens.
    if (assignment.feeToken == address(0)) {
        // Paying Ether
        _blk.assignedProver.sendEther(proverFee, MAX_GAS_PAYING_PROVER);
    } else {
        // Paying ERC20 tokens
        IERC20(assignment.feeToken).safeTransferFrom(
            _meta.coinbase, _blk.assignedProver, proverFee
        );
    }

Notice how if the payment is in ERC20 tokens, the payee will be the variable _meta.coinbase, and like we showed earlier, this can be set to any arbitrary address by the proposer. This can lead to a scenario as such:

1. proposer A approves the assignmentHook contract to spend a portion of their tokens, the allowance is set higher than the actual fee they will be paying.
2. proposer A proposes a block, and a fee is charged and payed to the assigned prover, but there remains allowance that the assignment hook contract can still use.
3. proposer B proposes a block and sets params.coinbase as the the address of proposer A.
4. proposer A address will be the payee of the fee for the assigned prover for the block proposed by proposer B.

The scenario above describes how someone can be forced maliciously to pay fees for block proposals by other actors.

Tools Used

Manual Review

Recommended Mitigation Steps

A simple fix to this to ensure the block proposer will always be the msg.sender, as such:

    if (params.coinbase == address(0 || params.coinbase != msg.sender)) {
        params.coinbase = msg.sender;
    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/L1/libs/LibProving.sol#L221-L224

Vulnerability details

Impact

In Taiko, proving of blocks is done onchain, and is possible by calling the proveBlock function in TaikoL1 contract.

However there also exists a monitoring and alert system that continuously generates off-chain post-states. If a block transition leads to a different post-state than expected and is successfully proven, this indicates a potential issue or bug in the proving process. To address this, there are multiple tiers of oversight, with Tier N being the highest. In the event of a discrepancy, Tier N(Guardian Prover) has the authority to intervene and essentialy re-prove itself to set the correct post-state in the case its first proof was deemed invalid by offchain checks. This serves as a safeguard/failsafe to ensure the integrity of the proving process in all cases.

The problem is this mechanism to re prove a block by top tier will never work. Consider the code below:

 else {
        // New transition and old transition on the same tier - and if this transaction tries to
        // prove the same, it reverts
        if (sameTransition) revert L1_ALREADY_PROVED();

        if (isTopTier) {
            // The top tier prover re-proves.
            assert(tier.validityBond == 0);
            assert(ts.validityBond == 0 && ts.contestBond == 0 && ts.contester == address(0));

            ts.prover = msg.sender;
            ts.blockHash = _tran.blockHash;
            ts.stateRoot = _tran.stateRoot;

            emit TransitionProved({
                blockId: blk.blockId,
                tran: _tran,
                prover: msg.sender,
                validityBond: 0,
                tier: _proof.tier
            });
        }

This else block within the proveBlock function in the libProving library, will execute in the specific case where proving process occurs for a tier equal to the last tier proved, this will usually be the case when we want to either contest the current proof, or if the top tier wants to re-prove a block as a fail safe mechanism like we discussed earlier.

However the top tier re-prove will never work because of this check:

assert(ts.validityBond == 0 && ts.contestBond == 0 && ts.contester == address(0));

1. The validity bond in the top tier is indeed 0, so if the top tier intended to reprove itself, this statement is true.
2. the transition contester will indeed be the zero address, since the top tier is re-proving, so no contestation happened in last transition(contesting is not even possible at the top tier).
3. the contest bond of a transition by default will never be equal to 0. on the first proving it will be reset to 1, to save gas. so this condition will always fail. its set in the line 389: https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/L1/libs/LibProving.sol#L389

Based on the discussion above, the re proval failsafe will always fail. In the edge case that that there is a discrepancy in top tier proof as compared with the off chain check, there will be no way to reprove it.

Tools Used

Manual Review

Recommended Mitigation Steps

Update the assert as follows:

    assert(ts.validityBond == 0 && ts.contestBond == 1 && ts.contester == address(0));

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/automata-attestation/lib/PEMCertChainLib.sol#L135 https://github.com/code-423n4/2024-03-taiko/blob/0d081a40e0b9637eddf8e760fabbecc250f23599/packages/protocol/contracts/verifiers/SgxVerifier.sol#L115-L136

Vulnerability details

Impact

As part of of its ZK proof setup, Taiko leverages SGX provers. it also enables remote SGX attestation and this is possible via leveraging code from Automata, which provides a modular attestation layer extending machine-level trust to Ethereum via the AutomataDcapV3Attestation repo, which is in scope of this audit.

Anyone with SGX hardware can register their instance to be an SGX prover in the Taiko Network via calling the registerInstance function in SgxVerifier.sol. This is why attestation is critical to prove the reliability and trustworthiness of the SGX prover.

The attestation process of SGX provers is a multi fold process, and starts with calling the verifyParsedQuote function in AutomataDcapV3Attestation.sol. One of the steps involves decoding the certchain provided by the SGX prover, as seen below:

        // Step 4: Parse Quote CertChain
    IPEMCertChainLib.ECSha256Certificate[] memory parsedQuoteCerts;
    TCBInfoStruct.TCBInfo memory fetchedTcbInfo;
    {
        // 536k gas
        parsedQuoteCerts = new IPEMCertChainLib.ECSha256Certificate[](3);
        for (uint256 i; i < 3; ++i) {
            bool isPckCert = i == 0; // additional parsing for PCKCert
            bool certDecodedSuccessfully;
            // todo! move decodeCert offchain
            (certDecodedSuccessfully, parsedQuoteCerts[i]) = pemCertLib.decodeCert(
                authDataV3.certification.decodedCertDataArray[i], isPckCert
            );
            if (!certDecodedSuccessfully) {
                return (false, retData);
            }
        }
    }

after this step is executed, a number of other steps are done including:

Step 5: basic PCK and TCB check Step 6: Verify TCB Level Step 7: Verify cert chain for PCK Step 8: Verify the local attestation sig and qe report sig

The decoding of the certchain happens through the EMCertChainLib lib, and this involves a number of steps, one of which is to validate the decoded notBefore and notAfter tags of the certificate:

        {
        uint256 notBeforePtr = der.firstChildOf(tbsPtr);
        uint256 notAfterPtr = der.nextSiblingOf(notBeforePtr);
        bytes1 notBeforeTag = der[notBeforePtr.ixs()];
        bytes1 notAfterTag = der[notAfterPtr.ixs()];
        if (
            (notBeforeTag != 0x17 && notBeforeTag == 0x18)
                || (notAfterTag != 0x17 && notAfterTag != 0x18)
        ) {
            return (false, cert);
        }
        cert.notBefore = X509DateUtils.toTimestamp(der.bytesAt(notBeforePtr));
        cert.notAfter = X509DateUtils.toTimestamp(der.bytesAt(notAfterPtr));
    }

These fields determine the time format, whether the notBeforePtr and notAfterPtr are in UTC or generalized time, and are used to ensure consistency in timestamps used to determine the validity period of the certificate.

However the validation can fail because the logic above is faulty, as it will allow the attestor to pass in any value for the notBefore tag, indeeed the condition of:

(notBeforeTag != 0x17 && notBeforeTag == 0x18)

will allow the attestor to pass in any beforetag because the condition will always be false.

Consider if we pass an invalid tag of 0x10:

1. notBeforeTag != 0x17 is True.
2. notBeforeTag == 0x18 is False.
3. full condition is False.

I believe the original intention was to ensure the beforeTag is strictly 0x17 or 0x18, just as with the afterTag. Because of this oversight, a malicious attestor could pass in any notBefore Tag as part of their certificate.

This issue requires attention given the significance of the attestation process of SGX provers within Taiko's ZK setup. The whole point of attestation is to prove the SGX provers are secure, untampered, and trustworthy, and improper validation related to certificate validity periods can have unforeseen consequences.

Tools Used

Manual Review

Recommended Mitigation Steps

Update the condition as below:

            if (
            (notBeforeTag != 0x17 && notBeforeTag != 0x18)
                || (notAfterTag != 0x17 && notAfterTag != 0x18)
        ) {
            return (false, cert);

Assessed type

Invalid Validation