GAP variable inconsistency in Bridge.sol

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/a30b5b6afd121e4de8ceff7165a2091e62194992/packages/protocol/contracts/bridge/IBridge.sol#L60-L64
https://github.com/code-423n4/2024-03-taiko/blob/a30b5b6afd121e4de8ceff7165a2091e62194992/packages/protocol/contracts/bridge/Bridge.sol#L29-L48

Vulnerability details

Impact

Possible storage collision
Impact - High
Chances - Low to medium
Severity - medium

Proof of Concept

__gap variable is used in upgradeble contracts to ensure that any new variable if added in parent or child contract does not collide with any other previously used storage slot. Hence its size must clearly defined, which Taiko contracts do except in Bridge.sol , in the mentioned file they have assumed that Context struct uses 3 slots but it only uses 2 slots.

    struct Context {
        bytes32 msgHash; // Message hash.
        address from; // 20 bytes
        uint64 srcChainId; ///@audit 8 bytes , 20 + 8 bytes will be packed in 1 slot
    }

This wrong assumption has lead to incorrect allotment size to __gap array, which should be 44 instead of 43.

It can also be verified by running

forge inspect contracts/bridge/Bridge.sol:Bridge storage --pretty

Tools Used

Manual Review

Recommended Mitigation Steps

    /// @notice The next message ID.
    /// @dev Slot 1.
    uint128 public nextMessageId;

    /// @notice Mapping to store the status of a message from its hash.
    /// @dev Slot 2.
    mapping(bytes32 msgHash => Status status) public messageStatus;

--    /// @dev Slots 3, 4, and 5.
++    /// @audit Slots 3,4
    Context private __ctx;

    /// @notice Mapping to store banned addresses.
--    /// @dev Slot 6.
++    /// @audit slot 5
    mapping(address addr => bool banned) public addressBanned;

    /// @notice Mapping to store the proof receipt of a message from its hash.
--    /// @dev Slot 7.
++    /// @audit slot 6
    mapping(bytes32 msgHash => ProofReceipt receipt) public proofReceipt;

--    uint256[43] private __gap;
++    uint256[44] private __gap;

Some real world hacks and previous findings explaining why consistent gap size is important-
audius hack , finding 1 , finding 2

Assessed type

Upgradable

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[dantaik]

Fixed in https://github.com/taikoxyz/taiko-mono/pull/16594/files

[c4-sponsor]

dantaik marked the issue as disagree with severity

[c4-sponsor]

dantaik (sponsor) confirmed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b