The decision to return the liveness bond depends solely on the last guardian

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/provers/GuardianProver.sol#L46
https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProving.sol#L192

Vulnerability details

The GuardianProver contract is a multisig that might contest any proof in some exceptional cases (bugs in the prover or verifier). To contest a proof, a predefined number of guardians should approve a hash of the message that includes _meta and _tran.

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

The issue arises from the fact that the approved message doesn't include the _proof. It means that the last approving guardian can provide any desired value in the _proof. The data from the _proof is used to determine whether it is necessary to return the liveness bond to the assigned prover or not:

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

As a result, the last guardian can solely decide whether to return the liveness bond to the assigned prover or not.

Impact

The decision to return the liveness bond depends solely on the last guardian.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider including the _proof in the approved message.

bytes32 hash = keccak256(abi.encode(_meta, _tran, _proof));

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-pre-sort]

minhquanym marked the issue as duplicate of #248

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[PaperParachute]

Comment on sponsor behalf (content from private Discord channel):

Now guardian provers must also agree on whether liveness bond should be returned bytes32(_proof.data) == LibStrings.H_RETURN_LIVENESS_BOND, so the hash they sign is now:

bytes32 hash = keccak256(abi.encode(_meta, _tran, _proof.data));

rather than the previous code:

bytes32 hash = keccak256(abi.encode(_meta, _tran));