Taiko - ladboy233's results

Lines of code

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

Vulnerability details

lack of input validation when proposing block which can make the assigned prover loss money

Line of code

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

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

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

Proof of concept

in function proposeBlock,

Line of code

 function proposeBlock(
        TaikoData.State storage _state,
        TaikoData.Config memory _config,
        IAddressResolver _resolver,
        bytes calldata _data,
        bytes calldata _txList
    )

in the end of proposeBlock, the assigned prover needs to transfer 250 token as bond via the assignment hook

// Run all hooks.
            // Note that address(this).balance has been updated with msg.value,
            // prior to any code in this function has been executed.
            address prevHook;
            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);
            }

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

the expect hook and the expected called function is:

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

which calls onBlockProposed function,

and in the function note these lines. Line of code

// Send the liveness bond to the Taiko contract
        IERC20 tko = IERC20(resolve("taiko_token", false));
        tko.transferFrom(_blk.assignedProver, taikoL1Address, _blk.livenessBond);

        // Find the prover fee using the minimal tier
        uint256 proverFee = _getProverFee(assignment.tierFees, _meta.minTier);

        // The proposer irrevocably pays a fee to the assigned prover, either in
        // Ether or ERC20 tokens.
        if (assignment.feeToken == address(0)) {
            // Paying Ether
            _blk.assignedProver.sendEther(proverFee, MAX_GAS_PAYING_PROVER);
        } else {
            // Paying ERC20 tokens
            IERC20(assignment.feeToken).safeTransferFrom(
                _meta.coinbase, _blk.assignedProver, proverFee
            );
        }

Impact

note the logic:

else {
// Paying ERC20 tokens
IERC20(assignment.feeToken).safeTransferFrom(
    _meta.coinbase, _blk.assignedProver, proverFee
);

We transfer the fee token from coinbase address to the assigend prover address,

the problem is that:

the fee token, proverFee amount and coinbase address lacks input validation:

function onBlockProposed(
        TaikoData.Block memory _blk,
        TaikoData.BlockMetadata memory _meta,
        bytes memory _data
    )
        external
        payable
        nonReentrant
        onlyFromNamed("taiko")
    {
        // Note that
        // - 'msg.sender' is the TaikoL1 contract address
        // - 'block.coinbase' is the L1 block builder
        // - 'meta.coinbase' is the L2 block proposer

        Input memory input = abi.decode(_data, (Input));

as we can see, the fee token comes from the decoded data:

Line of code

struct ProverAssignment {
        address feeToken;
        uint64 expiry;
        uint64 maxBlockId;
        uint64 maxProposedIn;
        bytes32 metaHash;
        bytes32 parentMetaHash;
        TaikoData.TierFee[] tierFees;
        bytes signature;
    }

    struct Input {
        ProverAssignment assignment;
        uint256 tip; // A tip to L1 block builder
    }

Problem is that the user can set coinbase address to any address.

A malicious user can set:

fee token to taiko token,

and coinbase address to a previous assigned prover who can give max approval to the hook contract.

if (params.coinbase == address(0)) {
   params.coinbase = msg.sender;
}

Then, the taiko token is transferred from the previous assigned prover

to the current assigned prover via the hook, which makes the previous assigned prover lose fund

while another address that approve the hook as a spender also lose fund.

One of the most possible case is that if multiple prover uses the same hook address,

they need to give approval to the hook contract

to spend their taiko token to pay for the bond fee when propsoing the block

Recommendation

Implement comprehensive validation checks

for all inputs involved in the block proposal process.

This includes verifying the legitimacy and integrity of the

fee token, the prover fee amount, and the coinbase address.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Lack of signature replay protection in TimelockTimePool.sol withdraw function

Line of code

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

Proof of concept

In TimelockTokenPool.sol,

Line of code

  /// @notice Withdraws all withdrawable tokens.
  /// @param _to The address where the granted and unlocked tokens shall be sent to.
  /// @param _sig Signature provided by the grant recipient.
  // @audit
  // replay protection is missing
  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);
  }

Impact

there is lack of validation for signature replay,

then once the recipient sign a signature, anyone can replay the signature to trigger withdraw repeatedly

Line of code

  function _withdraw(address _recipient, address _to) private {
        Recipient storage r = recipients[_recipient];

        (,,, uint128 amountToWithdraw, uint128 costToWithdraw) = getMyGrantSummary(_recipient);

        r.amountWithdrawn += amountToWithdraw;
        r.costPaid += costToWithdraw;

        totalAmountWithdrawn += amountToWithdraw;
        totalCostPaid += costToWithdraw;

        IERC20(taikoToken).transferFrom(sharedVault, _to, amountToWithdraw);
        IERC20(costToken).safeTransferFrom(_recipient, sharedVault, costToWithdraw);

And this will let the sharedVault recipient replay the amountToWithdraw several times,

and force the _recipient to pay the costToWithdraw several times to drain the fund

that remain static in the _recipient wallet

Recommendation

mark the hash as used and do not allow signature reuse.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

User can select a time window when the block.baseFee is high to collect high fee from deposit fee when processing deposit

Line of code

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

Proof of concept

Inside the function processDeposits,

When the deposit is processed,

this line of code is executed, Line of code

// @audit this can changed? the block.basefee
uint96 fee = uint96(_config.ethDepositMaxFee.min(block.basefee * _config.ethDepositGas)); 

the fee is computed using the formula:

block.basefee * _config.ethDepositGas

Impact

the _config.ethDepositGas is hardcoded as 21000

but the block.baseFee depends on the block usage

Refer to the EIP-1559 Gas Fee calculations doc

The Base Fee is determined by the Ethereum network rather than being set by end-users looking to transact or miners seeking to validate transactions. The Base Fee targets 50% full blocks and is based upon the contents of the most recent confirmed block. Depending on how full that new block is, the Base Fee is automatically increased or decreased.

For example:  

If the last block was exactly 50% full, the Base Fee will remain unchanged. 
If the last block was 100% full, the Base Fee will increase by the maximum 12.5% for the next block.
If the last block was more than 50% full but less than 100% full, the Base Fee will increase by less than 12.5%.
If the last block was 0% full – that is, empty –  the Base fee will decrease the maximum 12.5% for the next block.
If the last block was more than 0% full but less than 50% full, the Base Fee will decrease by less than 12.5%  

note that _config.ethDepositMaxFee is 1 ether / 10 = 0.1 ETH

then this actually only give user incentive to selectively process the deposit

when the block usage is very high to collect high fee from deposit fund.

and user basically ignore the deposit and cease the process when the block.baseFee is low.

Recommendation

Introduce a maximum fee cap that limits the deposit fee regardless of the baseFee

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Banned address can still receive message when a failed message is retried

Line of code

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

Proof of concept

When process the message in Bridge contract,

if a address is banned, the message is marked DONE.

Line of code

      // Process message differently based on the target address
      if (
          _message.to == address(0) || _message.to == address(this)
              || _message.to == signalService || addressBanned[_message.to]
      ) {
          // Handle special addresses that don't require actual invocation but
          // mark message as DONE
          refundAmount = _message.value;
          _updateMessageStatus(msgHash, Status.DONE);
      }

Impact

But the same addressBanned check is missing when the message is retried.

Assume the following sequence of event.

1. A user send a cross-chain message to an address
2. Message is processed, but failed first time and the message is marked as Status.RETRIABLE
3. admin blocklist and ban the address
4. Message is retried, message is executed on a banned address

Recommendation

we need to validate if an address is banned when retry message

and refund ETH and fee and mark the message as DONE

if the address is banned when address is retried.

   function retryMessage(
        Message calldata _message,
        bool _isLastAttempt
    )
        external
        nonReentrant
        whenNotPaused
        sameChain(_message.destChainId)
    {
        if (_message.gasLimit == 0 || _isLastAttempt) {
            if (msg.sender != _message.destOwner) revert B_PERMISSION_DENIED();
        }

        bytes32 msgHash = hashMessage(_message);
        if (messageStatus[msgHash] != Status.RETRIABLE) {
            revert B_NON_RETRIABLE();
        }

        // Attempt to invoke the messageCall.
        if (_invokeMessageCall(_message, msgHash, gasleft())) {
            _updateMessageStatus(msgHash, Status.DONE);
        } else if (_isLastAttempt) {
            _updateMessageStatus(msgHash, Status.FAILED);
        }
        emit MessageRetried(msgHash);
    }

Recommendation

validate if the target address is banned when message is retried.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Lack of strict validation for gas limit because of 1 / 64 rules allow cross-chain transaction to be executed in gas limit that is less than use specify

Line of code

1. https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L217
2. https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L282
3. https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L491
4. https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md

Proof of concept

When sending cross chain from source chain to target chain,

anyone can process the message permissionlessly if the gaslimit is set to a non-zero value Line of code

function processMessage(
        Message calldata _message,
        bytes calldata _proof
    )
        external
        nonReentrant
        whenNotPaused
        sameChain(_message.destChainId)
    {

then inside the function we call these lines of code

  // Use the specified message gas limit if called by the owner, else
  // use remaining gas
  uint256 gasLimit = msg.sender == _message.destOwner ? gasleft() : _message.gasLimit;

  if (_invokeMessageCall(_message, msgHash, gasLimit)) {
      _updateMessageStatus(msgHash, Status.DONE);
  } else {
      _updateMessageStatus(msgHash, Status.RETRIABLE);
  }

the gas limit is passed into the function _invokeMessageCall

Line of code

function _invokeMessageCall(
        Message calldata _message,
        bytes32 _msgHash,
        uint256 _gasLimit
    )
        private
        returns (bool success_)
    {
        if (_gasLimit == 0) revert B_INVALID_GAS_LIMIT();
        assert(_message.from != address(this));

        _storeContext(_msgHash, _message.from, _message.srcChainId);

        if (
            _message.data.length >= 4 // msg can be empty
                && bytes4(_message.data) != IMessageInvocable.onMessageInvocation.selector
                && _message.to.isContract()
        ) {
            success_ = false;
        } else {
            (success_,) = ExcessivelySafeCall.excessivelySafeCall(
                _message.to,
                _gasLimit,
                _message.value,
                64, // return max 64 bytes
                _message.data
            );
        }

Impact

But Because

refer to eip-150 specification

if a call asks for more gas than all but one 64th of the maximum allowed amount, 

call with all but one 64th of the maximum allowed 

amount of gas (this is equivalent to a version of EIP-90ethereum/EIPs#90 plus EIP-114ethereum/EIPs#114). 

CREATE only provides all but one 64th of the parent gas to the child call.

the 1/64 rules presences, only the 63/64 gas from gas limit is passed and spent to execute the message

then malicious user can ensure that the 63 / 64 of gasleft() is less than the message.gasLimit to make message executes but failed

The following POC shows that lack of validation on the gas limit results in failed transaction

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "forge-std/console.sol";

contract ExternalContract {
    
    function entry() public {

        try this.execute() {
            uint256 cur = gasleft();
            for (uint i = 0; i < 3650; i++) {
                i += 10;
                // console.log("i: ", i);
            }
            console.log("gas used: ", cur - gasleft());
        } catch {
            
        } 

    }

    function execute() public {
        console.log("gas left: ", gasleft());
    }

}

contract CounterTest is Test {

    using stdStorage for StdStorage;
    StdStorage stdlib;

    function setUp() public {

    }

    function testEIP150() public {
        ExternalContract externalContract = new ExternalContract();

        uint256 gasLimit = 64000;
    
        externalContract.entry{gas: gasLimit}();
    }

    function testAccurateGasLimit() public {
        ExternalContract externalContract = new ExternalContract();

        uint256 gasLimit = 100000;
    
        externalContract.entry{gas: gasLimit}();
    }



}

running 3500 for loop would cost 63513 unit of gas

but if we specify gas limit as 64000, because only 63 / 64 of gas is passed to external call, the external transaction can still marked as failed

because this allows the external transaction executes in the gas limit less than user specify

Recommendation

The recommendation is check :

gasLeft() / 63 * 64 >= message.gasLimit()

to make sure that the gas spend on execute message is at least the message.gasLimit user requires

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Loss of processed fee when the ETH deposit as fee is processed

Line of code

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

Proof of concept

Line of code

function processDeposits(
        TaikoData.State storage _state,
        TaikoData.Config memory _config,
        address _feeRecipient
    )
        internal
        returns (TaikoData.EthDeposit[] memory deposits_)
    {
        // Calculate the number of pending deposits.
        uint256 numPending = _state.slotA.numEthDeposits - _state.slotA.nextEthDepositToProcess;
        if (numPending < _config.ethDepositMinCountPerBlock) {
            deposits_ = new TaikoData.EthDeposit[](0);
        } else {
            deposits_ =
                new TaikoData.EthDeposit[](numPending.min(_config.ethDepositMaxCountPerBlock));
            uint96 fee = uint96(_config.ethDepositMaxFee.min(block.basefee * _config.ethDepositGas)); // @audit this can changed? the block.basefee
            uint64 j = _state.slotA.nextEthDepositToProcess;
            uint96 totalFee;
            for (uint256 i; i < deposits_.length;) {
                uint256 data = _state.ethDeposits[j % _config.ethDepositRingBufferSize];
                deposits_[i] = TaikoData.EthDeposit({
                    recipient: address(uint160(data >> 96)),
                    amount: uint96(data),
                    id: j
                });
                uint96 _fee = deposits_[i].amount > fee ? fee : deposits_[i].amount;

                // 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
                // otherwise the function would be gassing out.
                unchecked {
                    deposits_[i].amount -= _fee;
                    totalFee += _fee;
                    ++i;
                    ++j;
                }
            }
            _state.slotA.nextEthDepositToProcess = j;
            // This is the fee deposit
            _state.ethDeposits[_state.slotA.numEthDeposits % _config.ethDepositRingBufferSize] =
                _encodeEthDeposit(_feeRecipient, totalFee);

            // Unchecked is safe:
            // - uint64 can store up to ~1.8 * 1e19, which can represent 584K
            // years if we are depositing at every second
            unchecked {
                _state.slotA.numEthDeposits++;
            }
        }
    }

After user deposit ETH, anyone can trigger processDeposits

the min amount ETH is 1 ETH per deposit

the max amount of ETH is 32 ETH per deposit

then when process the deposit, the fee is computed in this way:

 uint96 fee = uint96(_config.ethDepositMaxFee.min(block.basefee * _config.ethDepositGas)); // @audit this can changed? the block.basefee

Impact

the max fee is 1 ether / 10 = 0.1 ETH

the config.ethDepositGas = 21000

the block.baseFee changes depends on the block usage

but if we go to etherscan gas tracker, Ethereum gas tracker

a 15 gwei - 30 gwei is reasonable.

Let us assume 15 gwei,

using ethereum unit converter,

15 gwei = 0.000000015 ETH

15 gwei * 21000 = 0.000000015 ETH * 21000 = 0.00031499999999999996 ETH

We need to times 8

for (uint256 i; i < deposits_.length;) {

because the ethDepositMinCountPerBlock is 8

      ethDepositMinCountPerBlock: 8,
      ethDepositMaxCountPerBlock: 32,

0.00031499999999999996 ETH * 8 = 0.0025199999999999997 ETH

ok, when the block usage is not high, a user process 8 deposit and collect 0.00251 ETH as fee

_state.slotA.nextEthDepositToProcess = j;
// This is the fee deposit
_state.ethDeposits[_state.slotA.numEthDeposits % _config.ethDepositRingBufferSize] =
    _encodeEthDeposit(_feeRecipient, totalFee);

the totalFee is 0.00251 ETH

but the problem is that someone needs to process the ETH deposit, including the fee

then when later another user process the deposit,

assume the block.baseFee is high, the block.baseFee * 21000 exceed 0.1 ETH,

        uint96 fee = uint96(_config.ethDepositMaxFee.min(block.basefee * _config.ethDepositGas)); // @audit this can changed? the block.basefee

then when the previous user's ETH fee deposit transaction is processed, the 0.00251 ETH fee will all be paid as the fee

because 0.1 ETH > 0.00251 ETH

uint96 _fee = deposits_[i].amount > fee ? fee : deposits_[i].amount;

unchecked {
      deposits_[i].amount -= _fee;
      totalFee += _fee;
      ++i;
      ++j;
  }

Recommendation

Establish a minimum fee threshold to ensure that

the collected fees will always be sufficient

to cover the gas costs of processing,

regardless of block.baseFee fluctuations.

Assessed type

Token-Transfer