Taiko - joaovwfreire's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/L1/libs/LibProposing.sol#L108 https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/L1/libs/LibProposing.sol#L213 https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/L1/libs/LibProving.sol#L121

Vulnerability details

Description

The proposeBlock at the LibProposing library has the following check to ensure the proposed block has the correct parentMetaHash set:

function proposeBlock(
        TaikoData.State storage _state,
        TaikoData.Config memory _config,
        IAddressResolver _resolver,
        bytes calldata _data,
        bytes calldata _txList
    )
        internal
        returns (TaikoData.BlockMetadata memory meta_, TaikoData.EthDeposit[] memory deposits_)
    {
    ...
if (params.parentMetaHash != 0 && parentMetaHash != params.parentMetaHash) {
            revert L1_UNEXPECTED_PARENT();
        }
    ...
    }

However, there are no sanity checks to ensure params.parentMetaHash is not zero outside of the genesis block.

Impact

Malicious proposers can propose new blocks without any parentMetaHash. This can induce a maliciously-generated block to be artificially contested as the final block relies on data held by the meta_ variable. Snippet 1:

TaikoData.Block memory blk = TaikoData.Block({
            metaHash: keccak256(abi.encode(meta_)),
...
})

This also generates issues for independent provers, as they may not utilize the proposed block's data to attempt to prove it and utilize the correct parentMetaHash, which will make the LibProving:proveBlock call revert with an L1_BLOCK_MISTATCH error:

function proveBlock
	...
	if (blk.blockId != _meta.id || blk.metaHash != keccak256(abi.encode(_meta))) {
            revert L1_BLOCK_MISMATCH();
        }
	...
}

Also, according to the documentation,  If the parent block hash is incorrect, the winning transition won't be used for block verification, and the prover will forfeit their validity bond entirely. If a maliciously proposed block with zero parent block hash is contested and a higher-tier prover ends up proving the proposed block, then he/she loses its own validity bond.

Proof of Concept

The test suite contains the TaikoL1TestBase:proposeBlock that creates new block proposals with a zero parentMetaHash. This is called multiple times at tests like test_L1_verifying_multiple_blocks_once and test_L1_multiple_blocks_in_one_L1_block at the TaikoL1.t.sol test file, demonstrating the lack of reversion if the parentMetaHash is not zero outside of the genesis block.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to check the parentMetaHash value is not zero if it isn't at the genesis block, otherwise users are going to be able to wrongly induce contestations.

Assessed type

Invalid Validation

Lo-01 TaikoToken:init can be front-run

Description

The contract has no checks to provide initialization access-control. Therefore any party is able to initialize it and make Taiko deployer have to deploy a new contract from scratch. The impact is low due to the not-so-high costs for deploying new contracts, but it should be noted as the gas spent to deploy a new contract is much bigger than the amount to initialize it.

Recommended Mitigation Steps

Make sure to deploy the TaikoToken and initialize it atomically via a deployer contract, this way it is impossible to front-run the initialization. A second option is to batch the transactions at a MEV-protected transaction bundle, making the deployment and initialization of the contract effectively protected.

---

Lo-02 TaikoL2:anchor underflows block number making it revert when block number is zero

Description

On L2 genesis, the block.number is equal to zero. The init function at the TaikoL2 contract utilizes this value to calculate the publicInputHash global public bytes32. As can be seen at the following code snippet:

function init(
        address _owner,
        address _addressManager,
        uint64 _l1ChainId,
        uint64 _gasExcess
    )
        external
        initializer
    {
    ...
    if (block.number == 0) {
        // This is the case in real L2 genesis
    }
    ...
    (publicInputHash,) = _calcPublicInputHash(block.number);
    }

This makes calling the anchor transaction impossible at the same block init is called. The reason for that is during the anchor call, the parent block is calculated inside an unchecked block as block.number -1. In this case, 0 - 1 results in 2^256 - 1 for the first ever block.

function anchor(
        bytes32 _l1BlockHash,
        bytes32 _l1StateRoot,
        uint64 _l1BlockId,
        uint32 _parentGasUsed
    )
        external
        nonReentrant
    {
    ...
    uint256 parentId;
        unchecked {
            parentId = block.number - 1;
        }
    (bytes32 publicInputHashOld, bytes32 publicInputHashNew) = _calcPublicInputHash(parentId);
        if (publicInputHash != publicInputHashOld) {
            revert L2_PUBLIC_INPUT_HASH_MISMATCH();
        }
    ...
}

This will necessarily make the anchor call revert with the L2_PUBLIC_INPUT_HASH_MISMATCH error. The issue is the anchor call is always the first call of a block at Taiko L2 instance. Therefore the genesis block is not able to hold a single transaction.

---

Lo-03 TimelockTokenPool:getMyGrantSummary accepts precision mismatches

Description

The getMyGrantSummary view function is utilized for withdraw calculations at the TimelockTokenPool contract. It is crucial to determine the amount of taikoTokens and costTokens that will be withdrawn and payed respectively during withdraw calls. The issue lies when it rounds the amountUnlocked variable in order to induce precision accounting at the token level but withdraws tokens with wei level precision. Take a look at the following code snippet, it returns a amountToWithdraw variable that is not divided by anything and a costToWithdraw based on amounts unlocked that round down anything that is less than 1e18:

function getMyGrantSummary(address _recipient)
        public
        view
        returns (
            uint128 amountOwned,
            uint128 amountUnlocked,
            uint128 amountWithdrawn,
            uint128 amountToWithdraw,
            uint128 costToWithdraw
        )
    {
    ...
		amountWithdrawn = r.amountWithdrawn;
        amountToWithdraw = amountUnlocked - amountWithdrawn;

        // Note: precision is maintained at the token level rather than the wei level, otherwise,
        // `costPaid` must be a uint256.
        uint128 _amountUnlocked = amountUnlocked / 1e18; // divide first
        costToWithdraw = _amountUnlocked * r.grant.costPerToken - r.costPaid;
	}

This rounds down the amount paid per token while allowing more sub token amounts to be received without payment.

Impact

A user is able to withdraw sub 1e18 amounts while paying zero cost tokens. There's accumulation of dust amounts of up to 0.9999 token units per withdraw call.

Proof of Concept

Paste the following code snippet at the TimelockTokenPool.t.sol:

function test_precision_mismatch_1() public{
        uint64 grantStart = uint64(block.timestamp);
        uint32 grantPeriod = 1 days;
        pool.grant(
            Alice,
            TimelockTokenPool.Grant(1e18, strikePrice1, grantStart, 0, grantPeriod, 0, 0, 0)
        );

        vm.prank(Vault);
        tko.approve(address(pool), 1e18);

        uint256 halfTimeWithdrawCost = 5e17 / ONE_TKO_UNIT * strikePrice1;

        vm.warp(grantStart + 12 hours);
        (
            uint128 amountOwned,
            uint128 amountUnlocked,
            uint128 amountWithdrawn,
            uint128 amountToWithdraw,
            uint128 costToWithdraw
        ) = pool.getMyGrantSummary(Alice);

        assertEq(amountOwned, 5e17);
        assertEq(amountUnlocked, 5e17);
        assertEq(amountWithdrawn, 0);
        assertEq(amountToWithdraw, 5e17);
        assertEq(costToWithdraw, 0);

        vm.warp(grantStart + 1 days - 1);
        (amountOwned, amountUnlocked, amountWithdrawn, amountToWithdraw, costToWithdraw) =
            pool.getMyGrantSummary(Alice);
        assertApproxEqRel(amountOwned, 1e18, 1e15); // approximately 0.01% close
        assertApproxEqRel(amountUnlocked, 1e18, 1e15);
        assertEq(amountWithdrawn, 0);
        assertApproxEqRel(amountToWithdraw, 1e18, 1e15);
        assertEq(costToWithdraw, 0);

        vm.warp(grantStart + 1 days);
        (amountOwned, amountUnlocked, amountWithdrawn, amountToWithdraw, costToWithdraw) =
            pool.getMyGrantSummary(Alice);
        assertEq(amountOwned, 1e18);
        assertEq(amountUnlocked, 1e18);
        assertEq(amountWithdrawn, 0);
        assertEq(amountToWithdraw, 1e18);
        // notice how the cost to withdraw abruptly jumps
        assertEq(costToWithdraw, 1e4); // as it is 0.01 usdc per unit of tko

    }

Run it with the following command:

forge test --match-test test_precision_mismatch_1 -vvv

Tools Used

Manual review

Recommended Mitigation Steps

Divide amountToWithdraw by 1e18 then multiply it by 1e18 so it keeps the same entire token precision as used at the cost calculation.

---

Lo-04 ERC721Vault:onMessageInvocation allows minting to the Vault contract

Description

The ERC20Vault and ERC1155Vault have checks to ensure the vault contract is not the receiver on a transfer of tokens in the context of messageInvocation during bridging operations, as can be seen in the following code snippets: ERC20Vault:

function onMessageInvocation(bytes calldata _data)
        external
        payable
        nonReentrant
        whenNotPaused
    {
    ...
    if (to == address(0) || to == address(this)) revert VAULT_INVALID_TO();
    ...
}

ERC1155Vault:

function onMessageInvocation(bytes calldata data) external payable nonReentrant whenNotPaused {
...
	if (to == address(0) || to == address(this)) revert VAULT_INVALID_TO();
...

The ERC721Vault's onMessageInvocation function doesn't have the same check, therefore it allows tokens to be transferred the contract itself.

Recommended Mitigation Steps

Make sure to implement the same check at the ERC721Vault.onMessageInvocation function:

if (to == address(0) || to == address(this)) revert VAULT_INVALID_TO();

---

Lo-05 TimelockTokenPool:calcAmount private function returns the full amount if the start argument is equal zero.

Impact

Grants that are created with a zero value at either the grantStart or the unlockStart values don't return amounts proportional to how much time has passed in relation to grantPeriod/unlockPeriod, but rather the calcAmount private function returns the full amount if start is zero. Here is the code snippet outlining this behavior:

function _calcAmount(
        uint128 _amount,
        uint64 _start,
        uint64 _cliff,
        uint64 _period
    )
        private
        view
        returns (uint128)
    {
	...   
		if (_start == 0) return _amount;
	...
	}

Effectively this breaks the getAmountOwned and the getAmountUnlocked private view functions, as it returns the full grant amount even if the period has not fully passed. getAmountOwned is utilized by getMyGrantSummary, a function that determines the amount a grant recipient can withdraw at the withdraw function. It is also utilized by voidGrant private function to determine the amount to be voided. getAmountUnlocked is also utilized by getMyGrantSummary, and it also has a role in determining the amounts to withdraw. In practice this means all grants created with grantStart or unlockStart set to zero will be able to be fully voided or withdrawn right away.

Recommended Mitigation Steps

Remove the following line of the calcAmount private view function to ensure this wrong behavior is not possible anymore:

if (_start == 0) return _amount;

The final calcAmount method should look like this:

function _calcAmount(
        uint128 _amount,
        uint64 _start,
        uint64 _cliff,
        uint64 _period
    )
        private
        view
        returns (uint128)
    {
        if (_amount == 0) return 0;

        if (block.timestamp <= _start) return 0;

        if (_period == 0) return _amount; // @audit this is correct
        if (block.timestamp >= _start + _period) return _amount; // @audit this is correct

        if (block.timestamp <= _cliff) return 0;

        return _amount * uint64(block.timestamp - _start) / _period;
    }

Introduction

This analysis report's main intention is to provide insights on concerns that may represent threats to Taiko but don't necessarily fall into the vulnerabilities realm. In it I present ways trusted parties could provide risks to the protocol and informational findings. Note some of the findings may yield bad outcomes if not addressed, but considering those high or medium severity would be equivalent to stating footguns are valid issues. I have opted not to beat about the bush by diagramming or stating each of the smart contract's functions and variables as Taiko Labs already did so masterfully. Finally, I hope the read is enjoyable and the risks here presented provide value. The most important part of this report is at the Risks section: issues 1-4 are informational and 5-6 are centralization risks related to trusting too much on trusted parties.

Summary

Protocol

Brief Overview

Taiko is a Based Contestable Rollup (BCR). As straightforward as one can get, it means the rollup's architecture allows multi-party sequencing with contestations as the mechanism to ensure no wrong state transition is deferred.

Every time a sequencer tries to prove a previously proposed L2 state transition, the system requires a validity bond to incentivize non-malicious behavior. Contesting also comes with a bond cost to make sure disputes only occur when one is certain of a wrongly proposed state transition.

If the proof is contested, a sequencer with a higher tier than the first prover can present another proof that will either confirm the first sequencer's proof or deny it. Finally, rewards are distributed. * If the original proof is correct, the first-prover and the last one earn rewards and the contester loses its contestation bond. * Else, the first-prover loses its validity bond and the other parties get rewards.

The protocol maintains knowledge of the layer 1 state by anchoring updated layer 1 block details at the start of a new L2 block. This is what enables reliable cross-chain message verification.

The protocol also provides mechanisms to send ether and messages and transfer assets² to and from Layer 2 to Layer 1. ²-which is a bit more than what rollups commonly offer.

All those cross-chain messages/activities are mediated by the Bridge and the SignalService contracts. The bridge provides high-level entry points for users to interact with and its two main capabilities are sending messages (when a request is initiated) and processing those (when a relayer picks up the request and attempts to process them at the target chain). The other capabilities offered by the bridge are related to edge cases, such as retrying a message and calling it back to the source chain. The signal service provides the low-level logic for cross-chain message passing and is permissionless, even though it is not a good idea to call directly it without significant knowledge of the system. It defines methods for sending and verifying signals with Merkle proofs whose roots are dependent on proper layer 1 anchoring.

Sources

Taiko Labs has provided amazing content to enable developers and regular users to better understand Taiko. The docs are comprehensive and loaded with diagrams. I've selected some sources tailored for the technical-heavy audience as they have played a major role in understanding how the protocol works: Based Rollup FAQ — Taiko Labs (mirror.xyz) Based Contestable Rollup (BCR): A configurable, multi-proof roll… — Taiko Labs (mirror.xyz) Multi-proofs | Docs (taiko.xyz) Proposers and Provers on Taiko (Jolnir) (youtube.com) https://docs.taiko.xyz/core-concepts/bridging/

Methods

The security research was conducted in three steps: context-gathering, codebase-evaluation and report-generation.

Context-gathering: read all documentation available and review the sources provided at the protocol overview. Even though the documentation is solid, I have proudly asked many dumb questions and it helped to avoid submitting issues that were protocol design choices.

The objective during this phase is to develop a mental-model of flows and reach clarity on important concepts. This enables threat-modeling adjustments and helps classify the project into protocol categories to look for previously reported bugs. For this research, I have opted to study previous reports/bug bounties on Bridges, Rollups and Multi-Chain protocols on solodit.xyz and Jump Crypto.

Finally I read the already-available audit reports for Taiko and then started with the codebase.

Codebase-evaluation: consisted of 3 steps. First-glance, line-by-line review and semantical analysis.

1. First glance: I passively read the contracts before doing deep dives. This helps pointing the areas of most expected complexity and target those to be checked multiple times. If something looks odd or incomplete, I tag it with "// @audit" comments but don't develop the lead further for now. At the end of this stage, I have a decent understanding of the order of calls and expected flows, but no deep knowledge of execution traces.
2. Line-by-line review: the most thorough part of the process. Consists of analyzing the functions with as many details as possible. Even though I call this line-by-line, what actually happens is code-review by order of appearance. As an example: if we have a function a that calls function b before returning a value X + Y, I evaluate function b first then the correctness of X + Y. This is when most of the "// @audit" tags appear, as they represent leads that can be related, but not limited to: wrong external integrations, logic errors, poor state-management and invariants not being held. By the end of stage 2, I have gathered most of the leads worth testing.
3. Semantical analysis: consists on a more targeted approach. On Taiko I have attempted techniques like mirroring (looking at methods that did opposite things to find state asymmetries); repeated reads(reading the same complex portion of a function to check if all details are correctly handled) and random attack vectors(when one has an attack vector idea while running at the park).

Report-generation: involved gathering the observations made on the previous phases and pre-sorting them into the following categories: "ask-the-sponsor", "easy-proof-of-concept", "hard-proof-of-concept", "false-positive". Testing begins and a very small amount of leads actually makes it to the end. Finally, reports are categorized by severity level and, if necessary, placed at the Analysis category.

Risks

A-01 - TaikoToken:burn has centralization risks

Description

A burn function that allows a owner to arbitrarily burn tokens from any user may be utilized for censorship purposes. It also represents a important point of failure in case the owner's private keys fall into bad actors' possession.

Code snippet:

function burn(address _from, uint256 _amount) public onlyOwner {
        _burn(_from, _amount);
    }

A-02 - TaikoToken:snapshot has front-running risks

Description

The snapshot function at the TaikoToken serves for instant token balance accounting and can serve many purposes for a protocol. A malicious party may front-run snapshot transactions and acquire Taiko Tokens in order to inflate their accounted balances then sell back right after the snapshot is made.

Recommended Mitigation Steps

This risk is inevitable but can be lessened by avoiding the utilization of public mempools. Make sure to utilize MEV protection services for such transactions, such as Flashbots.

A-03 - mint is defined at the BridgedERC1155 contract but never called by the ERC1155Vault

Description

The BridgedERC1155 contract defines a mint method that is only callable by the erc1155_vault role, as shown in the following code snippet:

function mint(
        address _to,
        uint256 _tokenId,
        uint256 _amount
    )
        public
        nonReentrant
        whenNotPaused
        onlyFromNamed("erc1155_vault")
    {
        _mint(_to, _tokenId, _amount, "");
    }

The ERC1155Vault contract implementation, however, never calls this function, making it pointless.

A-04 - ERC1155Vault could use safeBatchTransferFrom instead of loop at the handleMessage private function

Description

The handleMessage private function runs a loop to do safeTransferFrom calls to an ERC1155 contract. Using individual safeTransferFrom calls is not optimal because the contract provides a safeBatchTransferFrom function designed for this scenario:

function _handleMessage(
        address _user,
        BridgeTransferOp memory _op
    )
        private
        returns (bytes memory msgData_, CanonicalNFT memory ctoken_)
    {
    ...
		for (uint256 i; i < _op.tokenIds.length; ++i) {
            IERC1155(_op.token).safeTransferFrom({
            from: msg.sender,
            to: address(this),
            id: _op.tokenIds[i],
            amount: _op.amounts[i],
            data: ""
	    });
    }
    ...
}

A-05 - TaikoL1:proveBlock allows a guardian to prove a block with maximum tier proof avoiding GuardianProver:approve's approval

Description

The proveBlock at the TaikoL1 contract allows a Guardian to call it directly with a topTier proof and bypass the GuardianProver approval flow that safeguards guardian proofs proving.

Impact

The GuardianProver contract becomes pointless, as anytime a Guardian may opt to bypass the approval flow that requires other guardians to agree with the proposed block and prove a block directly on its own.

Recommended Mitigation Steps

Make sure to check the caller of proveBlock is the GuardianProver contract if the proof tier is the highest. This can be done by adding the following lines of code at LibProving's proveBlock implementation:

function proveBlock(
        TaikoData.State storage _state,
        TaikoData.Config memory _config,
        IAddressResolver _resolver,
        TaikoData.BlockMetadata memory _meta,
        TaikoData.Transition memory _tran,
        TaikoData.TierProof memory _proof
    )
        internal
        returns (uint8 maxBlocksToVerify_)
    {
...
bool isTopTier = tier.contestBond == 0;
if (isTopTier){
	require(msg.sender == GUARDIAN_PROVER_CONTRACT);
}
...
}

A-06 GuardianProver:approve only verifies the proof argument of the caller that reaches a hash approval

Impact

Guardians are able to call the approve function at the GuardianProver contract without any proof.

Proof of Concept

Until approved, the approve function doesn't check the proofs sent by the guardians that vote for the approval of certain transition and its block metadata. Considering the function's purpose is to approve a guardian proof, then the proof ought to be evaluated, not only the transition. This behavior can be seen at the following code snippet, as only the transition and metadata are hashed and approved, but not the proof:

function approve(
        TaikoData.BlockMetadata calldata _meta,
        TaikoData.Transition calldata _tran,
        TaikoData.TierProof calldata _proof
    )
        external
        whenNotPaused
        nonReentrant
        returns (bool approved_)
    {
        if (_proof.tier != LibTiers.TIER_GUARDIAN) revert INVALID_PROOF();
        bytes32 hash = keccak256(abi.encode(_meta, _tran));
        approved_ = approve(_meta.id, hash);
...
}  

Recommended Mitigation Steps

Consider hashing the proof as well so that guardians agree on all ends of the proposed block:

bytes32 hash = keccak256(abi.encode(_meta, _tran, proof));
approved_ = approve(_meta.id, hash);

Time spent:

70 hours