# Taiko Audit

## Findings

### [Medium] `Address.isContract()` is not a reliable way of checking if the input is an EOA or not

**File(s)**: [`LibAddress.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/libs/LibAddress.sol)

**Description**: `isValidSignature` function in the library is used to check whether the signature is valid or not. It checks both EOA and smart contract wallets.
To identify the `addr` is contract or EOA it uses the openzeppelin's Address library's `isContract` method. This method is deprecated in latest version of [openzeppelin contracts]()

function isValidSignature(
    address addr,
    bytes32 hash,
    bytes memory sig
)
    internal
    view
    returns (bool valid)
{   
    // @audit do not use isContract method, it can be misused
    if (Address.isContract(addr)) {
        return IERC1271(addr).isValidSignature(hash, sig) == EIP1271_MAGICVALUE;
    } else {
        return ECDSA.recover(hash, sig) == addr;
    }
}

This method is deprecated in latest versions of openzeppelin contracts because misuse of it. This method returns `false` if the `addr` is EOA and `true` if the `addr` is contract by checking the code size stored in address. Problem is during the construction of a contract the code size is zero and if the method is called within a constructor of that contract then it will return false and classifies the contract address as EOA.

Severity is medium because if in future any new contract uses this library function in constructor, then it will fail it is logic.

**Recommendation(s)**: Consider using different method to check whether the `addr` is contract or not.

**Status**:

---

### [Medium] Lack of `__gap` variable

**File(s)**: [`SignalService.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/signal/SignalService.sol) [`TaikoToken.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/L1/TaikoToken.sol)

**Description**: The `SignalService` and `TaikoToken` contracts are intended to be used as logic contract with a proxy, but do not have a `__gap` variable. This would become problematic if a subsequent version was to inherit this contract. If the derived version were to have storage variables itself and additional storage variables were subsequently added to the inherited contract, a storage collision would occur.

**Recommendation(s)**: Consider adding a `__gap` variable (fixed number of slots (e.g 50)) as a private storage variable to allow the upgradable version to add more storage variables in future.

**Status**:

---

### [Low] Check `__gap`s have configure same size in all contracts

**File(s)**: [`Bridge.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/bridge/Bridge.sol) [`ERC20Vault.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/tokenvault/ERC20Vault.sol)

**Description**: Below contracts `__gap` variables size is not equal to 50. Standard size (50) should be followed, having different sizes isn't logical it is more difficult to maintain the code.

In `Bridge` contract, storage variables take 5 slots totally so size should be 45 instead of 44.

uint256[44] private __gap; // @audit can be changed to 45 from 44

In `ERC20Vault` contract, storage variables take 3 slots totally so size should be set to 47 instead of 46.

uint256[46] private __gap; // @audit can be changed to 47 from 46

**Recommendation(s)**: Consider changing the values of `__gap` variable array.

**Status**:

---

### [Low] `OwnableUpgradable` uses single-step ownership transfer

**File(s)**: [`OwnerUUPSUpgradable.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/common/OwnerUUPSUpgradable.sol)

**Description**: Using single step ownership transfer is not secure, existing owner can assign any new address as owner, and lose the ownership. Owner has more privileges, so this process should be taken care.
[`Ownable2StepUpgradable`](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/Ownable2StepUpgradeable.sol) provides added safety due to its securely designed two-step process to transfer the ownership.

**Recommendation(s)**: Consider using `Ownable2StepUpgradable` instead of `OwnableUpgradable`.

**Status**:

---

### [Low] Use `safeBatchTransferFrom` instead of `safeTransferFrom`

**File(s)**: [`ERC1155Vault.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/tokenvault/ERC1155Vault.sol)

**Description**: `receiveToken`, `onMessageRecalled` and `_handleMessage` functions in the contract use a for loop and safeTransferFrom method to transfer tokens. Using safeBatchTransferFrom method to transfer tokens will avoid a for loop and save gas.

function receiveToken(
    CanonicalNFT calldata ctoken,
    address from,
    address to,
    uint256[] memory tokenIds,
    uint256[] memory amounts
)
    external
    payable
    nonReentrant
    whenNotPaused
{
    ...
        if (ctoken.chainId == block.chainid) {
            // Token lives on this chain
            token = ctoken.addr;
            // @audit use safeBatchTransferFrom
            for (uint256 i; i < tokenIds.length; ++i) {
                ERC1155(token).safeTransferFrom({
                    from: address(this),
                    to: _to,
                    id: tokenIds[i],
                    amount: amounts[i],
                    data: ""
                });
            }
        } else {
    ...
}

**Recommendation(s)**: Consider using safeBatchTransferFrom instead of safeTransferFrom.

**Status**:

---

### [Low] Use `encodeCall` instead of `encodeWithSelector`

**File(s)**: [`ERC20Vault.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/tokenvault/ERC20Vault.sol) [`ERC1155Vault.sol`]() [`ERC721Vault.sol`]()

**Description**: `_encodeDestinationCall` private function uses `abi.encodeWithSelector` to wrap the `receiveToken` function call. This method doesn't provide type checking of function arguments.

function _encodeDestinationCall(
        address user,
        address token,
        address to,
        uint256 amount
    ) private returns (bytes memory msgData, uint256 _balanceChange) {
        ...
    // @audit use encodeCall instead of encodeWithSelector because the former provides type checking of arguments
        msgData = abi.encodeWithSelector(
            ERC20Vault.receiveToken.selector, ctoken, user, to, _balanceChange
    );
}

`abi.encodeCall` will check the arguments type and gives error if not same.

**Recommendation(s)**: Consider using `encodeCall` to generate message data for `receiveToken` function.

**Status**: Fixed

---

<!-- ### [Info] Usage of storage can be Minimized

**File(s)**: [`BridgedERC20.sol`](https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/contracts/tokenvault/BridgedERC20.sol)

**Description**: Current storage layout is like below, which uses 3 slots of storage and remaining is __gap (used when upgraded if new storage variables are added)

- srcToken (20 bytes) - slot0
- srcChainID (32 bytes) - slot1
- srcDecimals (1 bytes)- slot2

contract BridgedERC20 is EssentialContract, IMintableERC20, IERC20MetadataUpgradeable, ERC20Upgradeable {
    // @audit storage can be minimized
    address public srcToken;
    uint256 public srcChainId;
    uint8 private srcDecimals;
    
    uint256[47] private __gap;
    ...
}

Storage can be reduced to 2 slots instead of 3, if arranged like below

- srcDecimals (1 bytes)- slot0
- srcToken (20 bytes) - slot0
- srcChainID (32 bytes) - slot1

**Recommendation(s)**: Consider changing the layout of storage variables as recommended above.

**Status**:

--- -->

<!-- ### [High] Attacker can recallMessage in source chain after processMessage in destination chain

**File(s)**: [`Bridge.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/bridge/Bridge.sol)

**Description**:

**Recommendation(s)**: Consider to check whether the message status is DONE is destination chain before recalling the message.

**Status**:

--- -->

### [Best Practice] Remove unused errors and events

**File(s)**: [`Bridge.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/bridge/Bridge.sol) [`TaikoToken.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/L1/TaikoToken.sol)

**Description**: `Bridge` contract contains and unused error `B_INVALID_SIGNAL` and two unused events `SignalSent`, `DestChainEnabled`.

    ...
    event SignalSent(address indexed sender, bytes32 msgHash); // @audit unused event
    ...
    event DestChainEnabled(uint64 indexed chainId, bool enabled); // @audit unused event
    ...
    error B_INVALID_SIGNAL(); // @audit unused error
    ...

In `TaikoToken` contract, `TKO_INVALID_PREMINT_PARAMS` error is not used.

contract TaikoToken is EssentialContract, ERC20SnapshotUpgradeable, ERC20VotesUpgradeable {
    ...
    error TKO_INVALID_PREMINT_PARAMS(); // @audit unused error
    ...
}

**Recommendation(s)**: Consider removing the above unused error and events.

**Status**:

---

### [Best Practice] Check `oldAddress` and `newAddress` are not same

**File(s)**: [`AddressManager.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/common/AddressManager.sol)

**Description**: While setting a address the old address and new address can be same. To avoid spending unnecessary gas and emitting a new event for this, having a check before assignment will be a best practice.

function setAddress(
    uint64 chainId,
    bytes32 name,
    address newAddress
)
    external
    virtual
    onlyOwner
{   
    // @audit check oldAddress != newAddress before assigning
    address oldAddress = addresses[chainId][name]; 
    addresses[chainId][name] = newAddress;
    emit AddressSet(chainId, name, newAddress, oldAddress);
}

**Recommendation(s)**: Consider checking the `newAddress` before setting it.

**Status**:

---

### [Best Practice] Replace `assert` with `require`

**File(s)**: [`Bridge.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/bridge/Bridge.sol)

**Description**: `_invokeMessageCall` function in the contract uses assert to verify a condition, but this should not be used in deployment code as it is not a best practice to do.

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)); // @audit replace assert with require
    ...
}

**Recommendation(s)**: Consider replacing the `assert` statements with `require` statements in the contract.

**Status**:

---

### [Best Practice] Emit Events for all important operations

**File(s)**: [`Bridge.sol`](https://github.com/taikoxyz/taiko-mono/blob/based_contestable_zkrollup/packages/protocol/contracts/bridge/Bridge.sol)

**Description**: Four important functions within the Bridge contract are `sendMessage`, `processMessage`, `retryMessage` and `recallMessage`. Events are emitted for sendMessage and recallMessage which are `MessageSent` and `MessageRecalled` respectively. But no events emitted for other two functions.

**Recommendation(s)**: Consider emitting event for `processMessage` and `retryMessage` functions.

**Status**:

---