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); + } +}