Taiko - alexfilippov314's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L40

Vulnerability details

The current implementation of the ERC20Airdrop2 contract allows a user to claim funds between claimStart and claimEnd and to withdraw funds between claimEnd and claimEnd + withdrawalWindow. The withdrawable amount also grows linearly and becomes equal to the full claimAmount at the end of the withdrawal window. Presumably, it was implemented this way to deal with the tokens that might be never claimed.

However, several issues arise with the current implementation:

1. The only way to withdraw the full amount is to send a transaction that will be mined in the block with timestamp == claimEnd + withdrawalWindow. This block might not exist at all so the users might not be able to withdraw the full airdrop amount.
2. To minimize gas costs users are incentivized to send only one transaction as close to the end of the withdrawal window as possible. The issue here is that their transaction might be mined too late (because of the sudden gas spike or if the network is too busy) and they will not be able to withdraw anything.
3. From the UX perspective, some users might be forced to perform some actions or at least monitor the process (if the transaction will be executed on their behalf) at an inconvenient time (4 AM, for example).

Impact

The users might not be able to withdraw the full airdrop amount or could potentially lose it entirely.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider introducing a new storage variable withdrawalGrowPeriod that will be used in calculating the withdrawable amount. This value should be less than withdrawalWindow and provide a reasonable amount of time for users to execute the full withdrawal.

Assessed type

Timing

Lines of code

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

Vulnerability details

The TimelockTokenPool contract allows for token withdrawals on behalf of users by utilizing a permit-like signature:

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

This function might be called multiple times as the tokens are unlocked gradually. The issue arises from the fact that the signed message doesn't include deadline and nonce. It means that anyone can reuse this signature and replay a withdrawal transaction.

It's not much of an issue if the user manages approvals meticulously, but users often opt for unlimited approvals to save on gas. In this case, an attacker is still unable to steal anything, but they can replay a withdrawal transaction without the user's consent. This is quite concerning, as this transaction might essentially involve swapping costToken for taikoToken and the user might have some other use for costToken in mind. There also might be extreme cases where the user attempts to prevent liquidation using costToken, only to be front-run by the attacker.

This issue becomes even more severe if the _to address is compromised. In such a scenario, an attacker can get taikoToken at the expense of the user. The issue becomes more likely because of the following:

• Users might not think that the signature produced some time ago can be reused since such kinds of permits usually include nonce and deadline and can't be reused;
• Users have no means to revoke the signature, and the only way to protect themselves from this attack is to execute both costToken approval and withdrawal within a single transaction which is possible but isn't simple and obvious for the common user.

Impact

An attacker can reuse a signature to execute an action without the user's consent. In some cases it allows the attacker to steal user's funds.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a nonce and deadline to the signed message. Additionally, consider exposing a function that marks the current nonce as used, providing a means for the user to revoke signatures.

Assessed type

Invalid Validation

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

[1] This comment is misleading. The code does the opposite.

// Use the specified message gas limit if called by the owner, else
// use remaining gas

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

[2] This function is missing the onlyInitializing modifier:

function __Essential_init(address _owner) internal virtual {
    _transferOwnership(_owner == address(0) ? msg.sender : _owner);
    __paused = _FALSE;
}

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/common/EssentialContract.sol#L109

[3] There is a typo in the comment:

// Unchecked is safe:
// - _fee cannot be bigger than deposits_[i].amount
// - all values are in the same range (uint96) except loop
// counter, which obviously cannot be bigger than uint95 <- TYPO
// otherwise the function would be gassing out.

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

[4] The L1_TXLIST_OFFSET error is not defined in the TaikoErrors.sol. https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibProposing.sol#L54

[5] There is a typo (isAssignedPover) in this code block:

bool isAssignedPover = msg.sender == _blk.assignedProver;

// The assigned prover can only submit the very first transition.
if (_tid == 1 && _ts.tier == 0 && inProvingWindow) {
    if (!isAssignedPover) revert L1_NOT_ASSIGNED_PROVER();
} else {
    // Disallow the same address to prove the block so that we can detect that the
    // assigned prover should not receive his liveness bond back
    if (isAssignedPover) revert L1_ASSIGNED_PROVER_NOT_ALLOWED();
}

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

[6] Consider checking that the Config.ethDepositRingBufferSize is enough to accommodate Config.ethDepositMaxCountPerBlock deposits in the LibVerifying._isConfigValid function. https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/L1/libs/LibVerifying.sol#L245

[7] Multiple contracts have comments specifying how the addresses of these contracts are labeled in the AddressManager. However, the GuardianProver, GuardianVerifier, and SgxVerifier are missing this comment. Consider adding these comments to improve clarity and consistency.