Taiko - Tendency's results

Lines of code

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

Vulnerability details

Impact

TimelockTokenPool contract has two withdrawal functionality, one that allows withdrawal to self and another that allows withdrawal to a different address. The latter allows withdrawal to other addresses via a signature, it works by providing a signature and a receiving address on call, on the signature recovery, the returned signer acts as the recipient whose grants will be sent to the receiving _to address.

    function withdraw(address _to, bytes memory _sig) external {
        if (_to == address(0)) revert INVALID_PARAM();
        bytes32 hash = keccak256(abi.encodePacked("Withdraw unlocked Taiko token to: ", _to));
        address recipient = ECDSA.recover(hash, _sig);
        _withdraw(recipient, _to);
    }

The problem here is:

• Due to the lack of signature expiration or the use of a nonce value, the receiving address can continuously re-use the same signature to withdraw the recipient's accumulated grant before he can.

To Illustrate:

Consider the scenario where Alice, entitled to 200 grants, initially withdraws 100 for herself. She later decides to distribute 50 grants each to Bob and Charlie. Alice then signs a transaction to withdraw her current allowance of 50 grants to Bob. However, days later, when she intends to repeat the process for Charlie, Bob reuses Alice's previous transaction signature, to redirect her intended grant withdrawal to himself, leaving Charlie with either nothing or only a fraction of the intended amount, depending on Alice's remaining allowance at the time of execution. Bob can continually reuse the same signature to withdraw Alice's grants whenever she's allocated any in the future

• Final thoughts

From the current implementation, I believe whenever the recipient wishes to withdraw to self, she uses TimelockTokenPool::withdraw() and TimelockTokenPool::withdraw(_to, sig) when she intends to send her withdrawal to a different address. The receiving address could be owned or it might not, but regardless the signature should only be usable once to prevent this issue. Since the use of this functionality potentially puts the recipient's grants at risk of being stolen, I have labelled this issue to be of a High severity

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Either add a requirement that ensures the caller is the returned recipient or integrate a nonce value for signatures per recipient.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

When proving a block, Taiko has a tier system that allows provers to stake an x amount of validity bond as they provide proofs for a block, the higher the tier, the more bonds at stake. At the top of the tier we have the GUARDIAN TIER, this tier is in place to act as a watchdog and provide the correct proof, if the prior tier's proofs are wrong. To also correct a possible mistake from the GUARDIAN TIER, there is a condition in place that is meant to allow the top/guardian tier to override and thus set new proofs(block transition state).

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

• Let us now analyze the problem

Assuming the top tier calls TaikoL1::proveBlock but with a wrong stateRoot, down the logic, since this tier is higher than the previous tier, _overrideWithHigherProof function is invoked, after execution this function overrides the transition state to:

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

Note that, since this is the highest tier, the transition state validity bond and contest bond are expected to be zero:

        if (_tierId == LibTiers.TIER_GUARDIAN) {
            return ITierProvider.Tier({
                verifierName: "tier_guardian",
                validityBond: 0, // must be 0 for top tier
                contestBond: 0, // must be 0 for top tier
                cooldownWindow: 60, //1 hours
                provingWindow: 2880, // 48 hours
                maxBlocksToVerifyPerProof: 16
            });
        }

As you might have already noticed, the _overrideWithHigherProof function doesn't override the transition state contest bond to the tier contest bond, but instead initializes the contest bond as 1.

    _ts.contestBond = 1; // to save gas //

The effect is, on reproval(this will be when the guardians realize the used block state root is wrong), the current implementation expects the contest bond to be zero:

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

But since the contest bond has previously been initialized to 1(to save gas), this will thus result in a panic

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual Review

Recommended Mitigation Steps

The top tier is expected to have no contester and zero validity bond, thus, only checking those will be enough. Update to:

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

Assessed type

Error