Skip to content

Commit

Permalink
Fix: ToB-21 (unsafe casts) (#1457)
Browse files Browse the repository at this point in the history
* Add ChainContext library

* Add coverage for the new library

* Remove unsafe casts for getting chain context

* Safe cast for attestation nonces

* Make `stateIndex` uint8

* Adjust tests with uint8 stateIndex

* Add safe casts for 32 bits indexes

* Document dispute index unsafe cast

* Document unsafe casts in `memory` libs

* Document unsafe casts which are actually upcasts

* Document remaining unsafe casts

* Chore: better formatting

* Add SafeCast for `encodeTips256()`

* Bound gasDrop/gasLimit max values to avoid overflow in tests

* Revert changes that are already in #1430

* Better docs for snapshot merkle root

* Chore: explicit reverts when bounding in tests

* Add tests with potential context overflows
  • Loading branch information
ChiTimesChi authored Oct 30, 2023
1 parent 938a7be commit 9375970
Show file tree
Hide file tree
Showing 44 changed files with 365 additions and 91 deletions.
9 changes: 5 additions & 4 deletions packages/contracts-core/contracts/Destination.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {AGENT_ROOT_OPTIMISTIC_PERIOD} from "./libs/Constants.sol";
import {IndexOutOfRange, NotaryInDispute, OutdatedNonce} from "./libs/Errors.sol";
import {ChainGas, GasData} from "./libs/stack/GasData.sol";
import {AgentStatus, DestinationStatus} from "./libs/Structures.sol";
import {ChainContext} from "./libs/ChainContext.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {AgentSecured} from "./base/AgentSecured.sol";
import {DestinationEvents} from "./events/DestinationEvents.sol";
Expand Down Expand Up @@ -78,7 +79,7 @@ contract Destination is ExecutionHub, DestinationEvents, InterfaceDestination {
if (localDomain != synapseDomain) {
_nextAgentRoot = agentRoot;
InterfaceLightManager(address(agentManager)).setAgentRoot(agentRoot);
destStatus.agentRootTime = uint40(block.timestamp);
destStatus.agentRootTime = ChainContext.blockTimestamp();
}
// No need to do anything on Synapse Chain, as the agent root is set in BondingManager
}
Expand Down Expand Up @@ -198,12 +199,12 @@ contract Destination is ExecutionHub, DestinationEvents, InterfaceDestination {
{
status = destStatus;
// Update the timestamp for the latest snapshot root
status.snapRootTime = uint40(block.timestamp);
status.snapRootTime = ChainContext.blockTimestamp();
// No need to save agent roots on Synapse Chain, as they could be accessed via BondingManager
// Don't update agent root, if there is already a pending one
// Update the data for latest agent root only if it differs from the saved one
if (localDomain != synapseDomain && !rootPending && _nextAgentRoot != agentRoot) {
status.agentRootTime = uint40(block.timestamp);
status.agentRootTime = ChainContext.blockTimestamp();
status.notaryIndex = notaryIndex;
_nextAgentRoot = agentRoot;
emit AgentRootAccepted(agentRoot);
Expand All @@ -224,7 +225,7 @@ contract Destination is ExecutionHub, DestinationEvents, InterfaceDestination {
if (GasData.unwrap(gasData) == GasData.unwrap(storedGasData.gasData)) continue;
// Save the gas data
_storedGasData[domain] =
StoredGasData({gasData: gasData, notaryIndex: notaryIndex, submittedAt: uint40(block.timestamp)});
StoredGasData({gasData: gasData, notaryIndex: notaryIndex, submittedAt: ChainContext.blockTimestamp()});
}
}
}
3 changes: 2 additions & 1 deletion packages/contracts-core/contracts/Summit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {Receipt, ReceiptLib} from "./libs/memory/Receipt.sol";
import {Snapshot, SnapshotLib} from "./libs/memory/Snapshot.sol";
import {AgentFlag, AgentStatus, DisputeFlag, MessageStatus} from "./libs/Structures.sol";
import {Tips, TipsLib} from "./libs/stack/Tips.sol";
import {ChainContext} from "./libs/ChainContext.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {AgentSecured} from "./base/AgentSecured.sol";
import {SummitEvents} from "./events/SummitEvents.sol";
Expand Down Expand Up @@ -276,7 +277,7 @@ contract Summit is SnapshotHub, SummitEvents, InterfaceSummit {
pending: true,
tipsAwarded: savedRcpt.tipsAwarded,
receiptNotaryIndex: rcptNotaryIndex,
submittedAt: uint40(block.timestamp)
submittedAt: ChainContext.blockTimestamp()
});
// Save receipt tips
_receiptTips[messageHash] = ReceiptTips({
Expand Down
5 changes: 3 additions & 2 deletions packages/contracts-core/contracts/base/MessagingBase.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

// ══════════════════════════════ LIBRARY IMPORTS ══════════════════════════════
import {ChainContext} from "../libs/ChainContext.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {MultiCallable} from "./MultiCallable.sol";
import {Versioned} from "./Version.sol";
Expand All @@ -27,8 +29,7 @@ abstract contract MessagingBase is MultiCallable, Versioned, Ownable2StepUpgrade
uint256[50] private __GAP; // solhint-disable-line var-name-mixedcase

constructor(string memory version_, uint32 synapseDomain_) Versioned(version_) {
// TODO: do we want to/need to check for overflow?
localDomain = uint32(block.chainid);
localDomain = ChainContext.chainId();
synapseDomain = synapseDomain_;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ abstract contract StatementInboxEvents {
* @param attPayload Raw payload with Attestation data for snapshot
* @param attSignature Notary signature for the attestation
*/
event InvalidStateWithAttestation(uint256 stateIndex, bytes statePayload, bytes attPayload, bytes attSignature);
event InvalidStateWithAttestation(uint8 stateIndex, bytes statePayload, bytes attPayload, bytes attSignature);

/**
* @notice Emitted when a proof of invalid state in the signed snapshot is submitted.
* @param stateIndex Index of invalid state in the snapshot
* @param snapPayload Raw payload with snapshot data
* @param snapSignature Agent signature for the snapshot
*/
event InvalidStateWithSnapshot(uint256 stateIndex, bytes snapPayload, bytes snapSignature);
event InvalidStateWithSnapshot(uint8 stateIndex, bytes snapPayload, bytes snapSignature);

/**
* @notice Emitted when a proof of invalid state report is submitted.
Expand Down
14 changes: 9 additions & 5 deletions packages/contracts-core/contracts/hubs/ExecutionHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {Request} from "../libs/stack/Request.sol";
import {SnapshotLib} from "../libs/memory/Snapshot.sol";
import {AgentFlag, AgentStatus, MessageStatus} from "../libs/Structures.sol";
import {Tips} from "../libs/stack/Tips.sol";
import {ChainContext} from "../libs/ChainContext.sol";
import {TypeCasts} from "../libs/TypeCasts.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {AgentSecured} from "../base/AgentSecured.sol";
Expand All @@ -36,6 +37,7 @@ import {IExecutionHub} from "../interfaces/IExecutionHub.sol";
import {IMessageRecipient} from "../interfaces/IMessageRecipient.sol";
// ═════════════════════════════ EXTERNAL IMPORTS ══════════════════════════════
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

/// @notice `ExecutionHub` is a parent contract for `Destination`. It is responsible for the following:
Expand All @@ -52,6 +54,7 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec
using MessageLib for bytes;
using ReceiptLib for bytes;
using SafeCall for address;
using SafeCast for uint256;
using TypeCasts for bytes32;

/// @notice Struct representing stored data for the snapshot root
Expand Down Expand Up @@ -115,7 +118,7 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec
bytes memory msgPayload,
bytes32[] calldata originProof,
bytes32[] calldata snapProof,
uint256 stateIndex,
uint8 stateIndex,
uint64 gasLimit
) external nonReentrant {
// This will revert if payload is not a formatted message payload
Expand Down Expand Up @@ -150,7 +153,7 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec
// This is the first valid attempt to execute the message => save origin and snapshot proof
rcptData.origin = header.origin();
rcptData.rootIndex = rootData.index;
rcptData.stateIndex = uint8(stateIndex);
rcptData.stateIndex = stateIndex;
if (success) {
// This is the successful attempt to execute the message => save the executor
rcptData.executor = msg.sender;
Expand Down Expand Up @@ -284,13 +287,14 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec
function _saveAttestation(Attestation att, uint32 notaryIndex, uint256 sigIndex) internal {
bytes32 root = att.snapRoot();
if (_rootData[root].submittedAt != 0) revert DuplicatedSnapshotRoot();
// TODO: consider using more than 32 bits for the root index
_rootData[root] = SnapRootData({
notaryIndex: notaryIndex,
attNonce: att.nonce(),
attBN: att.blockNumber(),
attTS: att.timestamp(),
index: uint32(_roots.length),
submittedAt: uint40(block.timestamp),
index: _roots.length.toUint32(),
submittedAt: ChainContext.blockTimestamp(),
sigIndex: sigIndex
});
_roots.push(root);
Expand Down Expand Up @@ -347,7 +351,7 @@ abstract contract ExecutionHub is AgentSecured, ReentrancyGuardUpgradeable, Exec
bytes32 msgLeaf,
bytes32[] calldata originProof,
bytes32[] calldata snapProof,
uint256 stateIndex
uint8 stateIndex
) internal view returns (SnapRootData memory rootData) {
// Reconstruct Origin Merkle Root using the origin proof
// Message index in the tree is (nonce - 1), as nonce starts from 1
Expand Down
14 changes: 5 additions & 9 deletions packages/contracts-core/contracts/hubs/SnapshotHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {ChainGas, GasData, GasDataLib} from "../libs/stack/GasData.sol";
import {MerkleMath} from "../libs/merkle/MerkleMath.sol";
import {Snapshot, SnapshotLib} from "../libs/memory/Snapshot.sol";
import {State, StateLib} from "../libs/memory/State.sol";
import {ChainContext} from "../libs/ChainContext.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {AgentSecured} from "../base/AgentSecured.sol";
import {SnapshotHubEvents} from "../events/SnapshotHubEvents.sol";
Expand Down Expand Up @@ -168,7 +169,7 @@ abstract contract SnapshotHub is AgentSecured, SnapshotHubEvents, ISnapshotHub {
}

/// @inheritdoc ISnapshotHub
function getSnapshotProof(uint32 attNonce, uint256 stateIndex) external view returns (bytes32[] memory snapProof) {
function getSnapshotProof(uint32 attNonce, uint8 stateIndex) external view returns (bytes32[] memory snapProof) {
if (attNonce == 0 || attNonce >= _notarySnapshots.length) revert NonceOutOfRange();
SummitSnapshot memory snap = _notarySnapshots[attNonce];
uint256 statesAmount = snap.statePtrs.length;
Expand Down Expand Up @@ -299,11 +300,6 @@ abstract contract SnapshotHub is AgentSecured, SnapshotHubEvents, ISnapshotHub {

// ══════════════════════════════════════════════ INTERNAL VIEWS ═══════════════════════════════════════════════════

/// @dev Returns the amount of saved _attestations (created from Notary snapshots) so far.
function _attestationsAmount() internal view returns (uint256) {
return _attestations.length;
}

/// @dev Checks if attestation was previously submitted by a Notary (as a signed snapshot).
function _isValidAttestation(Attestation att) internal view returns (bool) {
// Check if nonce exists
Expand Down Expand Up @@ -353,7 +349,7 @@ abstract contract SnapshotHub is AgentSecured, SnapshotHubEvents, ISnapshotHub {
}

/// @dev Returns indexes of agents who provided state data for the Notary snapshot with the given nonce.
function _stateAgents(uint32 nonce, uint256 stateIndex)
function _stateAgents(uint32 nonce, uint8 stateIndex)
internal
view
returns (uint32 guardIndex, uint32 notaryIndex)
Expand Down Expand Up @@ -431,8 +427,8 @@ abstract contract SnapshotHub is AgentSecured, SnapshotHubEvents, ISnapshotHub {
summitAtt.snapRoot = snapRoot;
summitAtt.agentRoot = agentRoot;
summitAtt.snapGasHash = snapGasHash;
summitAtt.blockNumber = uint40(block.number);
summitAtt.timestamp = uint40(block.timestamp);
summitAtt.blockNumber = ChainContext.blockNumber();
summitAtt.timestamp = ChainContext.blockTimestamp();
}

/// @dev Checks that an Attestation and its Summit representation are equal.
Expand Down
11 changes: 8 additions & 3 deletions packages/contracts-core/contracts/hubs/StateHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@ import {IncorrectOriginDomain} from "../libs/Errors.sol";
import {GasData, GasDataLib} from "../libs/stack/GasData.sol";
import {HistoricalTree} from "../libs/merkle/MerkleTree.sol";
import {State, StateLib} from "../libs/memory/State.sol";
import {ChainContext} from "../libs/ChainContext.sol";
// ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════
import {AgentSecured} from "../base/AgentSecured.sol";
import {StateHubEvents} from "../events/StateHubEvents.sol";
import {IStateHub} from "../interfaces/IStateHub.sol";
// ═════════════════════════════ EXTERNAL IMPORTS ══════════════════════════════
import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";

/// @notice `StateHub` is a parent contract for `Origin`. It is responsible for the following:
/// - Keeping track of the historical Origin Merkle Tree containing all the message hashes.
/// - Keeping track of the historical Origin States, as well as verifying their validity.
abstract contract StateHub is AgentSecured, StateHubEvents, IStateHub {
using SafeCast for uint256;
using StateLib for bytes;

struct OriginState {
Expand Down Expand Up @@ -93,7 +97,8 @@ abstract contract StateHub is AgentSecured, StateHubEvents, IStateHub {
/// @dev Returns nonce of the next sent message: the amount of saved States so far.
/// This always equals to "total amount of sent messages" plus 1.
function _nextNonce() internal view returns (uint32) {
return uint32(_originStates.length);
// TODO: consider using more than 32 bits for origin nonces
return _originStates.length.toUint32();
}

/// @dev Checks if a state is valid, i.e. if it matches the historical one.
Expand Down Expand Up @@ -135,8 +140,8 @@ abstract contract StateHub is AgentSecured, StateHubEvents, IStateHub {

/// @dev Returns a OriginState struct to save in the contract.
function _toOriginState() internal view returns (OriginState memory originState) {
originState.blockNumber = uint40(block.number);
originState.timestamp = uint40(block.timestamp);
originState.blockNumber = ChainContext.blockNumber();
originState.timestamp = ChainContext.blockTimestamp();
originState.gasData = _fetchGasData();
}

Expand Down
14 changes: 7 additions & 7 deletions packages/contracts-core/contracts/inbox/StatementInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem
/// @inheritdoc IStatementInbox
// solhint-disable-next-line ordering
function submitStateReportWithSnapshot(
uint256 stateIndex,
uint8 stateIndex,
bytes memory srSignature,
bytes memory snapPayload,
bytes memory snapSignature
Expand Down Expand Up @@ -107,7 +107,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem

/// @inheritdoc IStatementInbox
function submitStateReportWithAttestation(
uint256 stateIndex,
uint8 stateIndex,
bytes memory srSignature,
bytes memory snapPayload,
bytes memory attPayload,
Expand Down Expand Up @@ -138,7 +138,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem

/// @inheritdoc IStatementInbox
function submitStateReportWithSnapshotProof(
uint256 stateIndex,
uint8 stateIndex,
bytes memory statePayload,
bytes memory srSignature,
bytes32[] memory snapProof,
Expand Down Expand Up @@ -212,7 +212,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem

/// @inheritdoc IStatementInbox
function verifyStateWithAttestation(
uint256 stateIndex,
uint8 stateIndex,
bytes memory snapPayload,
bytes memory attPayload,
bytes memory attSignature
Expand All @@ -237,7 +237,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem

/// @inheritdoc IStatementInbox
function verifyStateWithSnapshotProof(
uint256 stateIndex,
uint8 stateIndex,
bytes memory statePayload,
bytes32[] memory snapProof,
bytes memory attPayload,
Expand Down Expand Up @@ -266,7 +266,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem
}

/// @inheritdoc IStatementInbox
function verifyStateWithSnapshot(uint256 stateIndex, bytes memory snapPayload, bytes memory snapSignature)
function verifyStateWithSnapshot(uint8 stateIndex, bytes memory snapPayload, bytes memory snapSignature)
external
returns (bool isValidState)
{
Expand Down Expand Up @@ -520,7 +520,7 @@ abstract contract StatementInbox is MessagingBase, StatementInboxEvents, IStatem
* @param state Typed memory view over the provided state payload
* @param snapProof Raw payload with snapshot data
*/
function _verifySnapshotMerkle(Attestation att, uint256 stateIndex, State state, bytes32[] memory snapProof)
function _verifySnapshotMerkle(Attestation att, uint8 stateIndex, State state, bytes32[] memory snapProof)
internal
pure
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface IExecutionHub {
bytes memory msgPayload,
bytes32[] calldata originProof,
bytes32[] calldata snapProof,
uint256 stateIndex,
uint8 stateIndex,
uint64 gasLimit
) external;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,5 @@ interface ISnapshotHub {
* @param stateIndex Index of state in the attestation's snapshot
* @return snapProof The snapshot proof
*/
function getSnapshotProof(uint32 attNonce, uint256 stateIndex) external view returns (bytes32[] memory snapProof);
function getSnapshotProof(uint32 attNonce, uint8 stateIndex) external view returns (bytes32[] memory snapProof);
}
Loading

0 comments on commit 9375970

Please sign in to comment.