From cb2f1c9e2ff8c0a1278ebb2717f9c17e3599c36d Mon Sep 17 00:00:00 2001 From: aelmanaa Date: Fri, 31 Jul 2026 09:25:12 +0100 Subject: [PATCH] fix: stop reporting outcomes the tooling did not observe Ten places where a script answered a question it had not asked, or reported success it had not established. They share one shape: a call that fails, or returns nothing, is read as a fact about the chain rather than as a failed read. The allowlist check is the sharpest. `checkAllowList` is a no-op while the allowlist is disabled, so it returns without reverting for every address, and reading that as "permitted" answered yes for anybody, 0x0 included, on the one command an operator runs to confirm a restriction is in force. It now reads the enforcement state first and says which of the three situations it found. The allowlist listing had the same ambiguity: an empty list means every sender is permitted when the hooks were deployed without one, and nobody may send when an enforced list was emptied. The listing now names the enforcement state outright, so an enforced empty list reads as what it is instead of being inferred from the absence of a warning. The pool deploys resolve decimals from the token when decimals() answers and from an explicit DECIMALS otherwise; decimals() is optional in ERC20 and the pool takes the value as a constructor argument by design, treating an on-chain read as a cross-check only. What changed is that the value is never guessed: with both sources present they must agree (the pool constructor's own InvalidDecimalArgs check, surfaced before the broadcast with a message that names the fix), and with neither the deploy stops instead of substituting something. The number is immutable and scales every amount the pool moves, so a wrong one is wrong in a way nothing downstream can detect. The transfer preflight simulated the receiver as the sender whenever ORIGINAL_SENDER was unset, which is the field the source pool gates on, so a clean GO could be followed by a live SenderNotAllowed. The sender it simulated is now printed with the inputs and named in the verdict. The rest: an allowlist update with neither list set broadcast an empty change and reported success; adopting a token that supplied no usable symbol stored it under the literal "unknown", where the next such token overwrote it, and the TOKEN_SYMBOL fallback covers a symbol() answering the empty string the same as one that reverts, since telling an operator to set a variable and then ignoring it is the same defect one layer down; the pool version resolver told operators their pool was not a pool when the read had several possible causes, and now separates that from the case where nothing is deployed at the address, which is definite; the roles audit accepted anything with code as a Safe, so a declared threshold of zero reconciled clean against a contract that answers no Safe getter; a lockbox whose token could not be read printed nothing at all; and the finality read printed an error and exited zero, telling any wrapper checking the exit code that the read had succeeded. --- docs/operations/pools.md | 22 ++-- script/config/AdoptToken.s.sol | 8 +- script/configure/GetLockBox.s.sol | 8 +- script/configure/allowlist/GetAllowList.s.sol | 39 ++++++- .../configure/allowlist/IsAllowListed.s.sol | 38 +++++++ .../configure/allowlist/UpdateAllowList.s.sol | 8 ++ .../finality-config/GetFinalityConfig.s.sol | 10 +- script/deploy/DeployBurnMintTokenPool.s.sol | 18 +-- .../deploy/DeployLockReleaseTokenPool.s.sol | 18 +-- script/diagnostics/PreflightTransfer.s.sol | 16 ++- script/utils/DeploymentUtils.s.sol | 105 +++++++++++++++++- script/utils/PoolVersion.s.sol | 26 ++++- src/roles/RolesAuditor.sol | 34 +++++- test/actions/PoolVersionDispatch.t.sol | 24 +++- test/config/AllowListReporting.t.sol | 60 ++++++++++ test/config/TokenSymbolResolution.t.sol | 73 ++++++++++++ test/deploy/TokenDecimalsResolution.t.sol | 70 ++++++++++++ 17 files changed, 524 insertions(+), 53 deletions(-) create mode 100644 test/config/AllowListReporting.t.sol create mode 100644 test/config/TokenSymbolResolution.t.sol create mode 100644 test/deploy/TokenDecimalsResolution.t.sol diff --git a/docs/operations/pools.md b/docs/operations/pools.md index f2e79bd..04b18a0 100644 --- a/docs/operations/pools.md +++ b/docs/operations/pools.md @@ -58,11 +58,14 @@ forge script \ The address is saved to `history/token-pools/{selectorName}/{timestamp}-{SYMBOL}-BurnMintTokenPool.json` (keys `{CHAIN_NAME_IDENTIFIER}_TOKEN_POOL` and `{CHAIN_NAME_IDENTIFIER}_TOKEN`). -Set `POOL_HOOKS=0x...` to attach an `AdvancedPoolHooks` contract at deploy time. Set `DECIMALS=` if -your token does not implement the optional `decimals()` ERC20 function; the script falls back to this -value and fails if neither is available. The script also attempts `grantMintAndBurnRoles` on the token -to grant the pool mint and burn rights; if the token does not implement it, the script prints -instructions to grant the roles manually. +Set `POOL_HOOKS=0x...` to attach an `AdvancedPoolHooks` contract at deploy time. The pool's decimals +value comes from the token's `decimals()` when it answers; `decimals()` is optional in ERC20, so for a +token without it set `DECIMALS=` explicitly - the pool takes the value as a constructor argument by +design and treats an on-chain read only as a cross-check. When both sources exist they must agree, and +when neither does the deploy stops: the value is immutable and scales every amount the pool moves, so +it is never guessed. The script also attempts `grantMintAndBurnRoles` on the token to grant the pool +mint and burn rights; if the token does not implement it, the script prints instructions to grant the +roles manually. ## Lock and release pool @@ -72,10 +75,11 @@ lockbox must authorize the pool before it can deposit or withdraw tokens. The de lockbox, then pool, then authorize the pool on the lockbox. `LOCK_BOX` is required and must be the address of a deployed `ERC20LockBox` for the token. Set -`POOL_HOOKS=0x...` to attach an already-deployed `AdvancedPoolHooks` contract. Set `DECIMALS=` if -your token does not implement `decimals()`. When deploying the lockbox, optionally set -`AUTHORIZED_CALLERS` (CSV or JSON array) to authorize addresses immediately, useful for letting the -deployer or token issuer deposit and withdraw initial liquidity. +`POOL_HOOKS=0x...` to attach an already-deployed `AdvancedPoolHooks` contract. Decimals resolve the same +way as for the BurnMint pool above: read from the token when `decimals()` answers, `DECIMALS=` for a +token without the optional getter, agreement required when both exist, refusal when neither. When +deploying the lockbox, optionally set `AUTHORIZED_CALLERS` (CSV or JSON array) to authorize addresses +immediately, useful for letting the deployer or token issuer deposit and withdraw initial liquidity. The golden-path targets `make deploy-lockbox CHAIN= VERIFY=1` and `make deploy-lockrelease-pool CHAIN= VERIFY=1` resolve the token and lock box from the registry; diff --git a/script/config/AdoptToken.s.sol b/script/config/AdoptToken.s.sol index df7c002..7d260bd 100644 --- a/script/config/AdoptToken.s.sol +++ b/script/config/AdoptToken.s.sol @@ -95,7 +95,13 @@ contract AdoptToken is Script { plan.chainId = chainId; plan.token = token; plan.pool = pool; - plan.tokenSymbol = DeploymentUtils._getSymbol(vm, token); + // The symbol keys every entry this adoption writes, so a defaulted one collides with any other + // token on this chain whose symbol could not be read, and the later adoption overwrites the + // earlier. Both look like ordinary entries afterwards, so require one that came from the token + // or from TOKEN_SYMBOL. + (bool symbolOk, string memory tokenSymbol) = DeploymentUtils._trySymbol(vm, token); + require(symbolOk, "The token supplied no usable symbol: set TOKEN_SYMBOL to the symbol to adopt it under"); + plan.tokenSymbol = tokenSymbol; console.log(""); console.log("========================================"); diff --git a/script/configure/GetLockBox.s.sol b/script/configure/GetLockBox.s.sol index 119fb33..1a76607 100644 --- a/script/configure/GetLockBox.s.sol +++ b/script/configure/GetLockBox.s.sol @@ -66,7 +66,13 @@ contract GetLockBox is Script { console.log(string.concat(" Token: ", vm.toString(tokenAddress), symbol)); console.log(string.concat(" Balance: ", vm.toString(balance))); - } catch {} + } catch { + // Catching silently would leave a lockbox whose token cannot be read looking like one + // with nothing worth printing. The lockbox address above holds either way, so name + // the part that is missing. + console.log(" Token: could not be read from the lockbox"); + console.log(" Balance: unknown (it is read through the token)"); + } } } catch (bytes memory err) { console.log(unicode"❌ Error: getLockBox() reverted."); diff --git a/script/configure/allowlist/GetAllowList.s.sol b/script/configure/allowlist/GetAllowList.s.sol index 66b932a..05aa895 100644 --- a/script/configure/allowlist/GetAllowList.s.sol +++ b/script/configure/allowlist/GetAllowList.s.sol @@ -41,10 +41,41 @@ contract GetAllowList is Script { console.log("========================================"); console.log(""); - address[] memory allowList = AdvancedPoolHooks(hooksAddress).getAllowList(); - console.log(string.concat("AllowList count: ", vm.toString(allowList.length))); - for (uint256 i = 0; i < allowList.length; i++) { - console.log(string.concat(" ", vm.toString(allowList[i]))); + // The list alone is ambiguous: an empty one means every sender is permitted when the hooks were + // deployed without an allowlist, and nobody may send when they were deployed with one and it was + // since emptied. Print the enforcement state so the list can be read. + bool enforced; + try AdvancedPoolHooks(hooksAddress).getAllowListEnabled() returns (bool enabled) { + enforced = enabled; + } catch { + console.log(unicode"❓ Could not read the allowlist state at this address."); + console.log(" Without it, an allowlist read here cannot be interpreted."); + console.log( + string.concat(" Confirm POOL_HOOKS is an AdvancedPoolHooks contract: ", vm.toString(hooksAddress)) + ); + console.log("========================================"); + console.log(""); + // The revert carries the failure into the exit code: printing an error and exiting 0 + // would tell any wrapper reading it that the allowlist state was read. + revert("getAllowListEnabled() could not be read (see above)"); + } + if (!enforced) { + console.log(unicode"⚠️ These hooks enforce NO allowlist: every sender is permitted."); + console.log(" Enforcement is fixed at deployment and cannot be turned on later. To restrict"); + console.log(" senders, deploy AdvancedPoolHooks with a non-empty ALLOWLIST and point the"); + console.log(" pool at it."); + } else { + // Name the enforcement state rather than leave it implied by the absence of the warning + // above: with it, a count of zero reads as what it is. + address[] memory allowList = AdvancedPoolHooks(hooksAddress).getAllowList(); + console.log("Allowlist enforcement: ON"); + console.log(string.concat("AllowList count: ", vm.toString(allowList.length))); + if (allowList.length == 0) { + console.log(unicode"⚠️ The list is enforced and empty: NO sender is permitted."); + } + for (uint256 i = 0; i < allowList.length; i++) { + console.log(string.concat(" ", vm.toString(allowList[i]))); + } } console.log("========================================"); console.log(string.concat("Pool Hooks: ", helperConfig.getExplorerUrl(chainId, "/address/", hooksAddress))); diff --git a/script/configure/allowlist/IsAllowListed.s.sol b/script/configure/allowlist/IsAllowListed.s.sol index 782b8dd..6d17954 100644 --- a/script/configure/allowlist/IsAllowListed.s.sol +++ b/script/configure/allowlist/IsAllowListed.s.sol @@ -39,6 +39,44 @@ contract IsAllowListed is Script { console.log("========================================"); console.log(""); + // Enforcement decides what a membership check can mean. `checkAllowList` is a no-op while the + // allowlist is disabled: it returns without reverting for every address, 0x0 included, so a + // non-revert says nothing until enforcement is established. Enforcement is fixed at deployment + // (`i_allowlistEnabled = allowlist.length > 0`, immutable), so hooks deployed with an empty + // allowlist can never enforce one. + bool enforced; + try AdvancedPoolHooks(hooksAddress).getAllowListEnabled() returns (bool enabled) { + enforced = enabled; + } catch { + console.log(unicode"❓ Could not read the allowlist state at this address."); + console.log(" Without it, nothing can be reported about CHECK_ADDRESS."); + console.log( + string.concat(" Confirm POOL_HOOKS is an AdvancedPoolHooks contract: ", vm.toString(hooksAddress)) + ); + console.log("========================================"); + console.log(""); + // The revert carries the failure into the exit code: printing an error and exiting 0 + // would tell any wrapper reading it that the allowlist state was read. + revert("getAllowListEnabled() could not be read (see above)"); + } + + if (!enforced) { + console.log( + unicode"⚠️ These hooks enforce NO allowlist: every sender is permitted, this one included." + ); + console.log(" Enforcement is fixed at deployment and cannot be turned on later. To restrict"); + console.log(" senders, deploy AdvancedPoolHooks with a non-empty ALLOWLIST and point the"); + console.log(" pool at it."); + console.log("========================================"); + console.log( + string.concat("Pool Hooks: ", helperConfig.getExplorerUrl(chainId, "/address/", hooksAddress)) + ); + console.log("========================================"); + console.log(""); + return; + } + + // Enforcement is on, so a revert now carries the membership answer and nothing else. bool isAllowListed = false; try AdvancedPoolHooks(hooksAddress).checkAllowList(checkAddress) { isAllowListed = true; diff --git a/script/configure/allowlist/UpdateAllowList.s.sol b/script/configure/allowlist/UpdateAllowList.s.sol index 94daa9e..408cf82 100644 --- a/script/configure/allowlist/UpdateAllowList.s.sol +++ b/script/configure/allowlist/UpdateAllowList.s.sol @@ -42,6 +42,14 @@ contract UpdateAllowList is EoaExecutor { address[] memory removes = HelperUtils._parseAddressArray(vm, vm.envOr("REMOVE_ADDRESSES", string("")), ""); address[] memory adds = HelperUtils._parseAddressArray(vm, vm.envOr("ADD_ADDRESSES", string("")), ""); + // With neither variable set both arrays parse empty, and the target accepts that as a no-op: the + // run would report success and link a real transaction, so a mistyped variable name would look + // like an applied change. Refuse before broadcasting. + require( + removes.length + adds.length > 0, + "No allowlist changes given. Set ADD_ADDRESSES and/or REMOVE_ADDRESSES (CSV or JSON array)." + ); + console.log(""); console.log("========================================"); console.log(unicode"📝 Update AllowList"); diff --git a/script/configure/finality-config/GetFinalityConfig.s.sol b/script/configure/finality-config/GetFinalityConfig.s.sol index 641ffd3..103b4bb 100644 --- a/script/configure/finality-config/GetFinalityConfig.s.sol +++ b/script/configure/finality-config/GetFinalityConfig.s.sol @@ -54,11 +54,17 @@ contract GetFinalityConfig is Script { try tokenPool.getAllowedFinalityConfig() returns (bytes4 allowedFinality) { FinalityConfigUtils._logFinalityConfig(allowedFinality); } catch (bytes memory err) { + // A revert here has several causes: a pool older than TokenPool 2.0.0, an address that is not + // a CCIP pool, or bytecode this run cannot execute. The raw data is printed so the reader can + // tell which, and the revert is re-raised rather than swallowed, because printing an error + // and exiting 0 tells any wrapper reading the exit code that the read succeeded. console.log( - unicode"❌ Error: getAllowedFinalityConfig() reverted. Pool may be v1 (requires TokenPool v2.0+)." + unicode"❌ Error: getAllowedFinalityConfig() reverted. The pool may predate TokenPool 2.0.0, this" ); - console.log(" Raw revert data:"); + console.log(" address may not be a CCIP token pool, or this run may not be able to execute its"); + console.log(" bytecode. Raw revert data:"); console.logBytes(err); + revert("getAllowedFinalityConfig() reverted (see raw revert data above)"); } // ── Footer ───────────────────────────────────────────────────────── diff --git a/script/deploy/DeployBurnMintTokenPool.s.sol b/script/deploy/DeployBurnMintTokenPool.s.sol index 837c182..24c5ded 100644 --- a/script/deploy/DeployBurnMintTokenPool.s.sol +++ b/script/deploy/DeployBurnMintTokenPool.s.sol @@ -6,12 +6,13 @@ import {HelperConfig} from "../HelperConfig.s.sol"; // Network configuration hel import {BurnMintTokenPool} from "@chainlink/contracts-ccip/contracts/pools/BurnMintTokenPool.sol"; import {CrossChainToken} from "@chainlink/contracts-ccip/contracts/tokens/CrossChainToken.sol"; import {IBurnMintERC20} from "@chainlink/contracts-ccip/contracts/interfaces/IBurnMintERC20.sol"; -import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {DeploymentUtils} from "../utils/DeploymentUtils.s.sol"; import {DeploymentRecorder} from "../utils/DeploymentRecorder.s.sol"; import {RegistryWriter} from "../../src/utils/RegistryWriter.sol"; /// @notice Deploys a BurnMint token pool for a token and records it in the address registry. +/// DECIMALS= is required only when the token does not answer decimals(), and must agree with it when +/// both exist; the resolved value is the pool's immutable scaling factor. contract DeployBurnMintTokenPool is Script { HelperConfig public helperConfig; @@ -46,14 +47,13 @@ contract DeployBurnMintTokenPool is Script { require(config.router != address(0), "Router not defined for this network"); require(config.rmnProxy != address(0), "RMN Proxy not defined for this network"); - // decimals() is optional in ERC20; fall back to DECIMALS env var if not present - uint8 decimals; - try IERC20Metadata(tokenAddress).decimals() returns (uint8 d) { - decimals = d; - } catch { - console.log(unicode"⚠️ decimals() not found on token, falling back to DECIMALS env var"); - decimals = uint8(vm.envUint("DECIMALS")); - } + // decimals() when the token answers, an explicit DECIMALS when it does not (the getter is + // optional in ERC20 and the pool takes the value as a constructor argument by design), a + // mismatch or a missing-on-both-sides refusal otherwise. Never a guess: the value is immutable + // and scales every amount the pool moves. + uint8 decimals = DeploymentUtils._resolveTokenDecimals( + vm, tokenAddress, vm.envOr("DECIMALS", DeploymentUtils.DECIMALS_UNSET) + ); // POOL_HOOKS alias > {CHAIN}_POOL_HOOKS > registry active.poolHooks. Optional (0x0 = no hooks). address poolHooks = vm.envOr("POOL_HOOKS", helperConfig.getDeployedPoolHooks(chainId)); diff --git a/script/deploy/DeployLockReleaseTokenPool.s.sol b/script/deploy/DeployLockReleaseTokenPool.s.sol index f826c35..ca966ca 100644 --- a/script/deploy/DeployLockReleaseTokenPool.s.sol +++ b/script/deploy/DeployLockReleaseTokenPool.s.sol @@ -5,12 +5,13 @@ import {Script, console} from "forge-std/Script.sol"; import {HelperConfig} from "../HelperConfig.s.sol"; // Network configuration helper import {LockReleaseTokenPool} from "@chainlink/contracts-ccip/contracts/pools/LockReleaseTokenPool.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; -import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {DeploymentUtils} from "../utils/DeploymentUtils.s.sol"; import {DeploymentRecorder} from "../utils/DeploymentRecorder.s.sol"; import {RegistryWriter} from "../../src/utils/RegistryWriter.sol"; /// @notice Deploys a LockRelease token pool (paired with an ERC20 LockBox) and records it in the registry. +/// DECIMALS= is required only when the token does not answer decimals(), and must agree with it when +/// both exist; the resolved value is the pool's immutable scaling factor. contract DeployLockReleaseTokenPool is Script { HelperConfig public helperConfig; @@ -50,14 +51,13 @@ contract DeployLockReleaseTokenPool is Script { require(config.router != address(0), "Router not defined for this network"); require(config.rmnProxy != address(0), "RMN Proxy not defined for this network"); - // decimals() is optional in ERC20; fall back to DECIMALS env var if not present - uint8 decimals; - try IERC20Metadata(tokenAddress).decimals() returns (uint8 d) { - decimals = d; - } catch { - console.log(unicode"⚠️ decimals() not found on token, falling back to DECIMALS env var"); - decimals = uint8(vm.envUint("DECIMALS")); - } + // decimals() when the token answers, an explicit DECIMALS when it does not (the getter is + // optional in ERC20 and the pool takes the value as a constructor argument by design), a + // mismatch or a missing-on-both-sides refusal otherwise. Never a guess: the value is immutable + // and scales every amount the pool moves. + uint8 decimals = DeploymentUtils._resolveTokenDecimals( + vm, tokenAddress, vm.envOr("DECIMALS", DeploymentUtils.DECIMALS_UNSET) + ); // POOL_HOOKS alias > {CHAIN}_POOL_HOOKS > registry active.poolHooks. Optional (0x0 = no hooks). address poolHooks = vm.envOr("POOL_HOOKS", helperConfig.getDeployedPoolHooks(chainId)); diff --git a/script/diagnostics/PreflightTransfer.s.sol b/script/diagnostics/PreflightTransfer.s.sol index 057d624..8a52e0e 100644 --- a/script/diagnostics/PreflightTransfer.s.sol +++ b/script/diagnostics/PreflightTransfer.s.sol @@ -45,6 +45,10 @@ interface IPoolToken { /// word, e.g. `0x0000000100000000000000000000000000000000000000000000000000000000`, to select the /// fast-finality inbound bucket), TOKEN_ARGS (bytes, IPoolV2 lock only). /// +/// ORIGINAL_SENDER is the account the source pool gates on (allowlist, policy engine). Left at its +/// default (RECEIVER) it asks whether the receiver could send to themselves, so set it whenever +/// someone else sends. Otherwise a clean GO here can still be followed by a live SenderNotAllowed. +/// /// Usage: /// make preflight SOURCE_CHAIN=ethereum-testnet-sepolia DEST_CHAIN=avalanche-fuji AMOUNT=10000 RECEIVER=0xYou /// # raw forge (the make recipe sets the two RPC URLs from each chain's rpcEnv): @@ -114,6 +118,7 @@ contract PreflightTransfer is Script, StdCheats { console.log(string.concat("Dest pool: ", vm.toString(ctx.destPool))); console.log(string.concat("Amount: ", vm.toString(ctx.amount))); console.log(string.concat("Receiver: ", vm.toString(ctx.receiver))); + console.log(string.concat("Sender: ", vm.toString(ctx.originalSender))); (bytes memory destPoolData, uint256 releaseAmount) = _simulateLockOrBurn(ctx); _simulateReleaseOrMint(ctx, destPoolData, releaseAmount); @@ -210,14 +215,14 @@ contract PreflightTransfer is Script, StdCheats { try IPoolV2(ctx.destPool).releaseOrMint(input, ctx.requestedFinality) returns ( Pool.ReleaseOrMintOutV1 memory out ) { - _go(out.destinationAmount); + _go(out.destinationAmount, ctx.originalSender); } catch (bytes memory reason) { _noGoRevert("destination releaseOrMint", ctx.destPool, localToken, reason); } } else { vm.prank(offRamp); try IPoolV1(ctx.destPool).releaseOrMint(input) returns (Pool.ReleaseOrMintOutV1 memory out) { - _go(out.destinationAmount); + _go(out.destinationAmount, ctx.originalSender); } catch (bytes memory reason) { _noGoRevert("destination releaseOrMint", ctx.destPool, localToken, reason); } @@ -248,9 +253,14 @@ contract PreflightTransfer is Script, StdCheats { } } - function _go(uint256 destinationAmount) internal pure { + /// @dev The verdict names the sender it simulated. The source pool gates on `originalSender` (the + /// allowlist, and any policy engine), so a GO holds for that account only: simulate one address + /// and send from another, and the same lane can strand on `SenderNotAllowed` straight after a + /// clean preflight. + function _go(uint256 destinationAmount, address originalSender) internal pure { console.log("========================================="); console.log(unicode"✅ GO: both pool legs simulate cleanly; this transfer would execute."); + console.log(string.concat(" simulated sender: ", vm.toString(originalSender))); console.log(string.concat(" destinationAmount (local decimals): ", vm.toString(destinationAmount))); } diff --git a/script/utils/DeploymentUtils.s.sol b/script/utils/DeploymentUtils.s.sol index ea80952..7dd18cb 100644 --- a/script/utils/DeploymentUtils.s.sol +++ b/script/utils/DeploymentUtils.s.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.24; import {Vm} from "forge-std/Vm.sol"; import {console} from "forge-std/console.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; +import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; /// @title DeploymentUtils /// @notice Shared deployment-saving utilities used by all deploy scripts to avoid duplication. @@ -211,10 +212,112 @@ library DeploymentUtils { /// `internal` so the single-writer `DeploymentRecorder` composes the registry key from the same /// symbol the ledger file is named with. function _getSymbol(Vm vm, address tokenAddress) internal view returns (string memory symbol) { + (, symbol) = _trySymbol(vm, tokenAddress); + } + + /// @notice The token's symbol, and whether one was established. + /// @dev `_getSymbol` ends at the literal "unknown" when no symbol was established. That is + /// serviceable for a filename, but it collides as a registry key: two unreadable tokens on one + /// chain both key as "unknown", and the later write overwrites the earlier. Callers that key + /// storage on the symbol use this variant and refuse when `ok` is false. + /// + /// `TOKEN_SYMBOL` covers both ways a token can fail to supply one: a `symbol()` that reverts and + /// a `symbol()` that answers the empty string. If it only covered the revert, a token answering + /// "" would be told to set `TOKEN_SYMBOL` and then have the value ignored. + /// @return ok True when a non-empty symbol other than the literal "unknown" came from the token or + /// from `TOKEN_SYMBOL`. The empty string and "unknown" count as no symbol wherever they came + /// from: both are what a failure looks like, so neither can key the registry. + /// @return symbol The symbol as established, which is "unknown" or empty when `ok` is false. + function _trySymbol(Vm vm, address tokenAddress) internal view returns (bool ok, string memory symbol) { + return _establishSymbol(_readSymbol(tokenAddress), vm.envOr("TOKEN_SYMBOL", string("unknown"))); + } + + /// @dev The raw on-chain read: a `symbol()` that reverts and one that is absent both come back as + /// the empty string, the same shape as a token answering "". + function _readSymbol(address tokenAddress) internal view returns (string memory symbol) { try IERC20Metadata(tokenAddress).symbol() returns (string memory s) { symbol = s; + } catch {} + } + + /// @dev The decision, separated from the reads so it can be pinned without `vm.setEnv` (which is + /// process-wide while tests run in parallel): the token's answer wins when non-empty, the + /// fallback covers everything else, and neither "" nor "unknown" counts from either source. + function _establishSymbol(string memory fromToken, string memory fallbackSymbol) + internal + pure + returns (bool ok, string memory symbol) + { + symbol = bytes(fromToken).length > 0 ? fromToken : fallbackSymbol; + ok = bytes(symbol).length > 0 && keccak256(bytes(symbol)) != keccak256(bytes("unknown")); + } + + /// @dev Sentinel meaning "DECIMALS was not supplied": no uint8 can hold it. + uint256 internal constant DECIMALS_UNSET = type(uint256).max; + + /// @notice The pool's token-decimals constructor argument. `decimals()` is optional in ERC20, and + /// the pool treats an on-chain read as a cross-check only (`TokenPool` verifies `localTokenDecimals` + /// against it when it answers, and skips the check when it does not), so a token without the getter + /// is a designed-for case: the operator supplies `DECIMALS` explicitly. The one thing this never + /// does is guess - the value is immutable and scales every amount the pool moves. + /// @param supplied The DECIMALS environment value, `DECIMALS_UNSET` when the variable is not set. + /// Read at the call site so the primitives catalog (and any reader of the deploy script) + /// sees the input where it is consumed. + function _resolveTokenDecimals(Vm vm, address tokenAddress, uint256 supplied) internal view returns (uint8) { + (bool okRead, uint8 read) = _readDecimals(tokenAddress); + if (!okRead && supplied != DECIMALS_UNSET) { + console.log( + string.concat( + unicode"⚠️ decimals() not readable on the token; using DECIMALS=", + vm.toString(supplied), + " as the pool's immutable scaling factor." + ) + ); + console.log(" Nothing on-chain can verify it - a wrong value mis-scales every transfer and"); + console.log(" only a pool redeploy can fix it."); + } + return _establishDecimals(okRead, read, supplied); + } + + /// @dev The raw on-chain read: absent and reverting `decimals()` look alike, per ERC20 both mean + /// "the token does not supply one". + function _readDecimals(address tokenAddress) internal view returns (bool ok, uint8 value) { + try IERC20Metadata(tokenAddress).decimals() returns (uint8 d) { + return (true, d); } catch { - symbol = vm.envOr("TOKEN_SYMBOL", string("unknown")); + return (false, 0); + } + } + + /// @dev The decision, pure so it is pinnable without `vm.setEnv`. `supplied == DECIMALS_UNSET` + /// means the DECIMALS variable was not set. + /// - read only: use the token's answer (zero-config path); + /// - supplied only: use the supplied value (the token has no getter, as ERC20 allows); + /// - both: they must agree - the same cross-check the pool constructor makes, surfaced here + /// with a message that names the fix (the pool's would be a raw `InvalidDecimalArgs`); + /// - neither: refuse, naming the variable to set. A guessed value would deploy a pool whose + /// immutable scaling factor nothing downstream can detect as wrong. + function _establishDecimals(bool okRead, uint8 read, uint256 supplied) internal pure returns (uint8) { + if (supplied == DECIMALS_UNSET) { + require( + okRead, + "The token does not answer decimals(): set DECIMALS= to the token's decimals to deploy its pool" + ); + return read; + } + require(supplied <= type(uint8).max, "DECIMALS must fit uint8 (0-255)"); + if (okRead) { + require( + uint256(read) == supplied, + string.concat( + "DECIMALS=", + Strings.toString(supplied), + " disagrees with the token's own decimals()=", + Strings.toString(read), + ": fix or drop the variable" + ) + ); } + return uint8(supplied); } } diff --git a/script/utils/PoolVersion.s.sol b/script/utils/PoolVersion.s.sol index 1da4ba3..7f4d304 100644 --- a/script/utils/PoolVersion.s.sol +++ b/script/utils/PoolVersion.s.sol @@ -260,17 +260,33 @@ library PoolVersion { try ITypeAndVersion(pool).typeAndVersion() returns (string memory t) { return t; } catch { - revert(_notAPool(pool)); + revert(_unreadable(pool)); } } - revert(_notAPool(pool)); + revert(_noCode(pool)); } - function _notAPool(address pool) private pure returns (string memory) { + /// @dev Nothing is deployed here, which is definite, and passing a token address is the usual cause. + function _noCode(address pool) private pure returns (string memory) { return string.concat( - "NotACcipTokenPool: no typeAndVersion() at ", + "NotACcipTokenPool: no contract at ", VM.toString(pool), - "; not a CCIP token pool. Did you pass the token address instead of the pool? See ", + ". Did you pass the token address instead of the pool? See ", + PoolVersions.DOCS, + "." + ); + } + + /// @dev A contract is deployed and it did not answer. "Not a pool" is only one of the reasons, so the + /// message stops short of choosing: an unrelated contract, a proxy pointing nowhere, and a pool + /// whose bytecode this run cannot execute all arrive here identically, and naming one of them + /// sends the reader to fix something that may be correct. + function _unreadable(address pool) private pure returns (string memory) { + return string.concat( + "NotACcipTokenPool: the contract at ", + VM.toString(pool), + " did not answer typeAndVersion(). It may not be a CCIP token pool, or it may be one this run" + " cannot execute (check evm_version against the pool's compiler target). See ", PoolVersions.DOCS, "." ); diff --git a/src/roles/RolesAuditor.sol b/src/roles/RolesAuditor.sol index 58d0cf5..c4b8f89 100644 --- a/src/roles/RolesAuditor.sol +++ b/src/roles/RolesAuditor.sol @@ -839,11 +839,28 @@ contract RolesAuditor { _fail("governance.safe.address", string.concat(VM.toString(safe), " has NO code on this chain")); return safe; } - _pass("governance.safe.address", string.concat(VM.toString(safe), " (has code)")); + // Code at an address is not a Safe. A declared safe.address that is really a timelock, or any + // other contract, answers neither getter, so both reads return zero and a declaration of + // `threshold: 0` with no owners reconciles PASS, though no Safe can hold a zero threshold. The + // probe is the one `RolesSnapshot` writes with, so the two sides cannot disagree about what + // counts as a Safe. + if (!RolesProbes._looksLikeSafe(safe)) { + _fail( + "governance.safe.address", + string.concat( + VM.toString(safe), + " has code but does not answer both getThreshold() and getOwners(), so it cannot be audited as a Safe" + ) + ); + return safe; + } + _pass("governance.safe.address", string.concat(VM.toString(safe), " (answers getThreshold() and getOwners())")); if (VM.keyExistsJson(json, ".roles.governance.safe.threshold")) { uint256 declared = VM.parseJsonUint(json, ".roles.governance.safe.threshold"); - (, uint256 live) = RolesProbes._tryUint(safe, "getThreshold()"); - if (declared == live) { + (bool ok, uint256 live) = RolesProbes._tryUint(safe, "getThreshold()"); + if (!ok) { + _fail("governance.safe.threshold", "getThreshold() could not be read, so nothing was compared"); + } else if (declared == live) { _pass("governance.safe.threshold", VM.toString(live)); } else { _fail( @@ -853,8 +870,15 @@ contract RolesAuditor { } } if (VM.keyExistsJson(json, ".roles.governance.safe.owners")) { - (, address[] memory owners) = RolesProbes._tryAddressArray(safe, abi.encodeWithSignature("getOwners()")); - _checkSet("governance.safe.owners", VM.parseJsonAddressArray(json, ".roles.governance.safe.owners"), owners); + (bool ok, address[] memory owners) = + RolesProbes._tryAddressArray(safe, abi.encodeWithSignature("getOwners()")); + if (!ok) { + _fail("governance.safe.owners", "getOwners() could not be read, so nothing was compared"); + } else { + _checkSet( + "governance.safe.owners", VM.parseJsonAddressArray(json, ".roles.governance.safe.owners"), owners + ); + } } } diff --git a/test/actions/PoolVersionDispatch.t.sol b/test/actions/PoolVersionDispatch.t.sol index a324d17..b506a1f 100644 --- a/test/actions/PoolVersionDispatch.t.sol +++ b/test/actions/PoolVersionDispatch.t.sol @@ -402,13 +402,27 @@ contract PoolVersionDispatchTest is Test { // Resolver: the four refusal classes, asserted on message content // ───────────────────────────────────────────────────────────────────────── - function test_Refusal_NotAPool() public { + /// @dev A contract is deployed here and it did not answer. Several things produce that, so the + /// refusal names the possibilities rather than picking one: telling an operator their pool is + /// not a pool sends them to change something that may be correct. + function test_Refusal_ContractPresentButDoesNotAnswer() public { address none = address(new NoTypeAndVersion()); string memory reason = _catchResolve(none); _assertContains(reason, "NotACcipTokenPool"); _assertContains(reason, vm.toString(none)); - _assertContains(reason, "not a CCIP token pool"); - _assertContains(reason, "token address instead of the pool"); + _assertContains(reason, "did not answer typeAndVersion()"); + _assertContains(reason, "evm_version"); + } + + /// @dev The two refusals must not read alike. One is a fact (nothing is deployed there), the other is + /// a list of candidates, and an operator who cannot tell them apart debugs the wrong one. + function test_Refusal_DistinguishesNoCodeFromNoAnswer() public { + string memory present = _catchResolve(address(new NoTypeAndVersion())); + string memory codeless = _catchResolve(address(uint160(uint256(keccak256("codeless-distinct"))))); + assertTrue( + keccak256(bytes(present)) != keccak256(bytes(codeless)), + "a deployed contract that stays silent and an empty address must not refuse identically" + ); } function test_Refusal_NotAPool_CodelessAddress() public view { @@ -417,7 +431,9 @@ contract PoolVersionDispatchTest is Test { address codeless = address(uint160(uint256(keccak256("codeless-address-fixture")))); string memory reason = _catchResolve(codeless); _assertContains(reason, "NotACcipTokenPool"); - _assertContains(reason, "not a CCIP token pool"); + _assertContains(reason, "no contract at"); + // Nothing is deployed, so the usual cause is worth naming here, unlike the has-code case. + _assertContains(reason, "token address instead of the pool"); (bool ok, PoolVersions.Version v,) = shim.tryResolve(codeless); assertFalse(ok, "codeless address degrades on read paths"); diff --git a/test/config/AllowListReporting.t.sol b/test/config/AllowListReporting.t.sol new file mode 100644 index 0000000..f84fd40 --- /dev/null +++ b/test/config/AllowListReporting.t.sol @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {Test} from "forge-std/Test.sol"; +import {AdvancedPoolHooks} from "@chainlink/contracts-ccip/contracts/pools/AdvancedPoolHooks.sol"; + +/// @notice What `checkAllowList` can and cannot tell a caller about an address. +/// +/// The call is a no-op while the allowlist is disabled: it returns without reverting for every address, +/// `0x0` included. A non-revert therefore carries a membership answer only once enforcement is known to +/// be on, and any report built on it has to establish that first. These tests pin the distinction. +contract AllowListReportingTest is Test { + address internal constant MEMBER = address(0xA11CE); + address internal constant STRANGER = address(0xB0B); + + function _hooks(address[] memory allowlist) private returns (AdvancedPoolHooks) { + return new AdvancedPoolHooks(allowlist, 0, address(0), new address[](0)); + } + + /// @dev The disabled case. `i_allowlistEnabled` is `allowlist.length > 0` and immutable, so hooks + /// deployed with no allowlist can never enforce one, and the check passes for anybody. + function test_CheckAllowList_IsNoOpWhileDisabled() public { + AdvancedPoolHooks hooks = _hooks(new address[](0)); + + assertFalse(hooks.getAllowListEnabled(), "an empty allowlist leaves enforcement off"); + + // Neither call means "this address is permitted": nothing is being checked at all. + hooks.checkAllowList(STRANGER); + hooks.checkAllowList(address(0)); + } + + /// @dev The enabled case, which is the only one where a non-revert carries information. + function test_CheckAllowList_EnforcesMembershipWhileEnabled() public { + address[] memory allowlist = new address[](1); + allowlist[0] = MEMBER; + AdvancedPoolHooks hooks = _hooks(allowlist); + + assertTrue(hooks.getAllowListEnabled(), "a non-empty allowlist enables enforcement"); + + hooks.checkAllowList(MEMBER); + vm.expectRevert(abi.encodeWithSignature("SenderNotAllowed(address)", STRANGER)); + hooks.checkAllowList(STRANGER); + } + + /// @dev Both states are reachable from the constructor, so a report that does not name which one it + /// observed cannot be acted on: the same "permitted" verdict means everything or nothing. + function test_AllowListState_OnlyGetAllowListEnabledSeparatesTheTwoStates() public { + AdvancedPoolHooks disabled = _hooks(new address[](0)); + address[] memory allowlist = new address[](1); + allowlist[0] = STRANGER; + AdvancedPoolHooks enabled = _hooks(allowlist); + + // checkAllowList answers identically for STRANGER in both, though only one permits it. + disabled.checkAllowList(STRANGER); + enabled.checkAllowList(STRANGER); + + assertFalse(disabled.getAllowListEnabled(), "disabled"); + assertTrue(enabled.getAllowListEnabled(), "enabled"); + } +} diff --git a/test/config/TokenSymbolResolution.t.sol b/test/config/TokenSymbolResolution.t.sol new file mode 100644 index 0000000..f4304b7 --- /dev/null +++ b/test/config/TokenSymbolResolution.t.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {Test} from "forge-std/Test.sol"; +import {DeploymentUtils} from "../../script/utils/DeploymentUtils.s.sol"; + +/// @dev A token whose `symbol()` answers, but with the empty string. ERC20 marks the function optional, +/// and "optional" covers this shape as well as the missing one. +contract EmptySymbolToken { + function symbol() external pure returns (string memory) { + return ""; + } +} + +/// @dev A token with no `symbol()` at all: the call reverts. +contract NoSymbolToken {} + +contract NamedToken { + function symbol() external pure returns (string memory) { + return "WIDGET"; + } +} + +/// @notice What `DeploymentUtils._trySymbol` accepts as a symbol, and what it refuses. +/// +/// The registry keys entries on the symbol, so anything that two different tokens could both resolve to +/// must not count as one: the empty string and the literal "unknown" are what a failure looks like, and +/// accepting either lets a later failed read overwrite an earlier token's entry. +/// +/// The decision is pinned through `_establishSymbol`, which takes the TOKEN_SYMBOL fallback as an +/// argument: `vm.setEnv` is process-wide while tests run in parallel, and DeployToken reads the same +/// variable, so these tests never touch the environment. +contract TokenSymbolResolutionTest is Test { + /// @dev A revert and an empty answer must reach the decision as the same shape, so the fallback + /// covers both. If the fallback covered only the revert, a token answering "" would be told to + /// set TOKEN_SYMBOL and then have the value ignored. + function test_ReadSymbol_RevertAndEmptyAnswerLookAlike() public { + assertEq(DeploymentUtils._readSymbol(address(new NoSymbolToken())), "", "a reverting symbol() reads empty"); + assertEq(DeploymentUtils._readSymbol(address(new EmptySymbolToken())), "", "an empty answer reads empty"); + assertEq(DeploymentUtils._readSymbol(address(new NamedToken())), "WIDGET"); + } + + function test_EstablishSymbol_TokenAnswerWinsOverFallback() public pure { + (bool ok, string memory symbol) = DeploymentUtils._establishSymbol("WIDGET", "FALLBACK"); + assertTrue(ok, "a non-empty symbol from the token is usable"); + assertEq(symbol, "WIDGET"); + } + + function test_EstablishSymbol_FallbackCoversAnEmptyRead() public pure { + (bool ok, string memory symbol) = DeploymentUtils._establishSymbol("", "FALLBACK"); + assertTrue(ok, "TOKEN_SYMBOL covers a token that supplied nothing"); + assertEq(symbol, "FALLBACK"); + } + + function test_EstablishSymbol_FailureShapesCountFromNeitherSource() public pure { + (bool okNothing,) = DeploymentUtils._establishSymbol("", "unknown"); + assertFalse(okNothing, "no answer and no fallback ends at the sentinel, which is no symbol"); + + (bool okEmptyFallback, string memory symbol) = DeploymentUtils._establishSymbol("", ""); + assertFalse(okEmptyFallback, "an empty TOKEN_SYMBOL is no symbol"); + assertEq(symbol, ""); + + (bool okSentinelFromToken,) = DeploymentUtils._establishSymbol("unknown", "FALLBACK"); + assertFalse(okSentinelFromToken, "a token whose symbol is the sentinel cannot key the registry"); + } + + /// @dev The composed read, on the one path that never consults the fallback. + function test_TrySymbol_SymbolFromToken_IsAccepted() public { + (bool ok, string memory symbol) = DeploymentUtils._trySymbol(vm, address(new NamedToken())); + assertTrue(ok); + assertEq(symbol, "WIDGET"); + } +} diff --git a/test/deploy/TokenDecimalsResolution.t.sol b/test/deploy/TokenDecimalsResolution.t.sol new file mode 100644 index 0000000..32c7866 --- /dev/null +++ b/test/deploy/TokenDecimalsResolution.t.sol @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {Test} from "forge-std/Test.sol"; +import {DeploymentUtils} from "../../script/utils/DeploymentUtils.s.sol"; + +/// @dev A token with no `decimals()` at all: the call reverts, which ERC20 permits. +contract NoDecimalsToken {} + +contract SixDecimalsToken { + function decimals() external pure returns (uint8) { + return 6; + } +} + +/// @notice How the pool deploys establish the token-decimals constructor argument. +/// +/// The pool stores the value immutable and scales every cross-chain amount with it, while `decimals()` +/// is optional in ERC20 and the pool itself treats an on-chain read as a cross-check only. So: the +/// token's answer when it gives one, an explicit DECIMALS when it does not, agreement required when +/// both exist, refusal when neither. The decision is pinned through `_establishDecimals`, which takes +/// the DECIMALS value as an argument: `vm.setEnv` is process-wide while tests run in parallel, and the +/// deploy fork tests read the same variable, so these tests never touch the environment. +contract TokenDecimalsResolutionTest is Test { + uint256 internal constant UNSET = DeploymentUtils.DECIMALS_UNSET; + + function test_ReadDecimals_AnswersAndAbsenceLookDistinct() public { + (bool okSix, uint8 six) = DeploymentUtils._readDecimals(address(new SixDecimalsToken())); + assertTrue(okSix); + assertEq(six, 6); + (bool okNone,) = DeploymentUtils._readDecimals(address(new NoDecimalsToken())); + assertFalse(okNone, "a token without the optional getter reads as unread, not as zero"); + } + + function test_EstablishDecimals_TokenAnswerAloneIsUsed() public pure { + assertEq(DeploymentUtils._establishDecimals(true, 6, UNSET), 6); + } + + function test_EstablishDecimals_SuppliedAloneIsUsed() public pure { + assertEq(DeploymentUtils._establishDecimals(false, 0, 9), 9, "DECIMALS covers a token without decimals()"); + } + + function test_EstablishDecimals_AgreementIsAccepted() public pure { + assertEq(DeploymentUtils._establishDecimals(true, 6, 6), 6); + } + + function test_EstablishDecimals_NeitherSourceRefuses() public { + vm.expectRevert( + bytes("The token does not answer decimals(): set DECIMALS= to the token's decimals to deploy its pool") + ); + this.establish(false, 0, UNSET); + } + + /// @dev The same cross-check the pool constructor makes (`InvalidDecimalArgs`), surfaced before + /// the broadcast with a message that names the fix. + function test_EstablishDecimals_MismatchRefuses() public { + vm.expectRevert(bytes("DECIMALS=8 disagrees with the token's own decimals()=6: fix or drop the variable")); + this.establish(true, 6, 8); + } + + function test_EstablishDecimals_OverflowRefusesInsteadOfTruncating() public { + vm.expectRevert(bytes("DECIMALS must fit uint8 (0-255)")); + this.establish(false, 0, 256); + } + + /// @notice External for `expectRevert`: an internal library call would revert the test itself. + function establish(bool okRead, uint8 read, uint256 supplied) external pure returns (uint8) { + return DeploymentUtils._establishDecimals(okRead, read, supplied); + } +}