BattleChain

BattleChainDeployer overrides each CreateX deployment function to call _registerDeployment(newContract) after invoking super. However, multiple convenience overloads in CreateX are implemented by calling another overload (for example, deployCreateAndInit(init, data, values) calls deployCreateAndInit(init, data, values, refundAddress), and deployCreate2(initCode) calls deployCreate2(salt, initCode)).

Because these internal calls are virtual, when BattleChainDeployer invokes super.deployCreate2(initCode), the CreateX implementation internally calls the deployCreate2(salt, initCode) variant, which is also overridden by the BattleChainDeployer. That override then deploys the contract and registers the contract once. Control then returns to the original one, which proceeds to call _registerDeployment again, resulting in duplicate registration and reverts.

As a result, most deployment functions in BattleChainDeployer always revert, since any overload that internally calls another overload attempts to register the same contract twice and fails with the ContractAlreadyRegistered error.

The failing variants are as follows:

• deployCreateAndInit(bytes memory initCode, bytes memory data, Values memory values)
• deployCreate2(bytes memory initCode)
• deployCreate2AndInit(bytes32 salt, bytes memory initCode, bytes memory data, Values memory values)
• deployCreate2AndInit(bytes memory initCode, bytes memory data, Values memory values, address refundAddress)
• deployCreate2AndInit(bytes memory initCode, bytes memory data, Values memory values)
• deployCreate2Clone(address implementation, bytes memory data)
• deployCreate3(bytes memory initCode)
• deployCreate3AndInit(bytes32 salt, bytes memory initCode, bytes memory data, Values memory values)
• deployCreate3AndInit(bytes memory initCode, bytes memory data, Values memory values, address refundAddress)
• deployCreate3AndInit(bytes memory initCode, bytes memory data, Values memory values)

Ensure that _registerDeployment is called only once per deployment. For overloads that delegate to another overload, consider removing the _registerDeployment call from the delegating wrapper to avoid duplicate registration.

The BountyTerms struct contains a diligenceRequirements string defining conditions whitehats must meet to claim bounties. The commitment-window checks in setBountyTerms guard five fields (bountyPercentage, bountyCapUsd, aggregateBountyCapUsd, identity, retainable) but not diligenceRequirements. During the commitment window, the owner can change this from empty to any arbitrary requirements, making bounty collection impossible.

As a result, whitehats who began work under no diligence requirements may be subjected to impossible conditions introduced mid-commitment, effectively preventing any bounty claims.

Block changes to diligenceRequirements during the commitment window.

The known-issues.md document states: “If an attack is discovered during the 3-day promotion delay, the attack moderator should call cancelPromotion (returning to UNDER_ATTACK) and then markCorrupted”. However, this behavior is not enforced in the contract.

markCorrupted can be called in both the UNDER_ATTACK and PROMOTION_REQUESTED states. If it is called directly during the PROMOTION_REQUESTED state without first cancelling the promotion, the agreement state becomes CORRUPTED, but the promotionRequestedTimestamp is not cleared. getAgreementInfo indicates that a promotion was requested and may be interpreted as still pending, even though the agreement has already been marked as corrupted.

Similarly, promotionRequestedTimestamp is not cleared when a terminal PRODUCTION state is reached. While this can serve as useful historical information about when the promotion was requested for that state, having a non-zero promotionRequestedTimestamp when the terminal state is CORRUPTED can be misleading and open to misinterpretation.

Consider enforcing a promotion cancellation before marking an agreement as corrupted, or clearing promotionRequestedTimestamp when the agreement reaches a terminal CORRUPTED state.

The commitment-window guard for aggregateBountyCapUsd in Agreement.setBountyTerms only prevents decreasing an existing non-zero cap. When the current value is 0 (no aggregate cap), new_value < 0 is always false for uint256, so introducing a brand-new aggregate cap passes the check. Adding a cap where none existed is unfavorable to whitehats as it limits the total payout pool across all participants.

Whitehats who evaluated the agreement under no-cap terms face a retroactively imposed payout ceiling. If multiple whitehats are active, the aggregate cap may prevent each from receiving their full individual cap.

Do not allow setting a new cap during the commitment window if the previous cap was zero.

During the commitment window, addAccounts is allowed because expanding scope is favorable to whitehats. However, there is no way to update the childContractScope of an existing account. For example, upgrading an account from None to All expands the attack surface, which is beneficial for whitehats. The only way to achieve this is to remove the account and re-add it with the new scope, but removeAccounts is blocked during the commitment window as an unfavorable change

function removeAccounts(string memory caip2ChainId, string[] memory accountAddresses) external onlyOwner {
    // Cannot remove accounts during commitment window
    if (block.timestamp < s_cantChangeUntil) {
        revert Agreement__CannotReduceScopeDuringCommitment();
    }
    // ...
}

As a result, a favorable scope expansion is unintentionally blocked.

Consider allowing childContractScope upgrades (toward more inclusive scopes) for existing accounts during the commitment window.

Agreement.setBountyTerms enforces that aggregateBountyCapUsd cannot decrease during the commitment window. Separately, _validateBountyTerms requires aggregateBountyCapUsd == 0 when retainable = true. These two constraints create a deadlock: an agreement with retainable = false and aggregateBountyCapUsd > 0 can never switch to retainable = true during the commitment window, because doing so requires setting aggregateBountyCapUsd to 0 (a decrease).

Making the bounty retainable is strictly favorable to whitehats as they keep recovered funds instead of returning them. The commitment window is designed to allow favorable changes, but this particular favorable transition is blocked.

As a result, agreement owners who initially chose an aggregate bounty cap cannot upgrade to retainable bounties during the commitment window despite being favorable to whitehats.

Allow aggregateBountyCapUsd to decrease to 0 when switching to retainable in the same call.

The initialize functions across multiple contracts perform duplicate address(0) checks, as outlined below:

• AttackRegistry.initialize checks that _initialOwner and _treasury are not address(0) and reverts if they are. However, the subsequent internal calls to __Ownable_init and _setTreasury already perform these validations.
• AgreementFactory.initialize does the same for _initialOwner and reverts before calling __Ownable_init(_initialOwner), even though __Ownable_init already reverts when the owner is zero.
• BattleChainSafeHarborRegistry.initialize checks whether owner == address(0) immediately before invoking __Ownable_init_unchained(owner), which already enforces this constraint.

These checks are therefore redundant and can be safely removed to reduce code duplication.

Consider removing redundant address(0) validation checks where the downstream initializer already enforces the same constraint. While retaining these checks is functionally harmless, removing them can reduce duplication.

The NatSpec comment on requestUnderAttack states that “the agreement owner must be the deployer of all contracts”. However, the function actually checks the authorized owner for each contract, rather than the deployer.

Since deployers can delegate ownership via authorizeAgreementOwner, the deployer does not necessarily have to match the agreement owner, making the comment inaccurate.

Update the comment to state that “the agreement owner must be the authorized owner for all contracts”.

Protocols adopting the BattleChain Safe Harbor deploy agreements and register them with both the AttackRegistry and the BattleChainSafeHarborRegistry. However, there is no enforcement or linkage between these two registries.

The BattleChain Safe Harbor documentation (https://docs.battlechain.com/battlechain/explanation/safe-harbor#verifying-protection) recommends this three-step check before attacking:

// 1. Agreement is valid
bool valid = safeHarborRegistry.isAgreementValid(agreementAddress);

// 2. Contract is attackable
bool attackable = attackRegistry.isTopLevelContractUnderAttack(contractAddress);

// 3. Contract is in scope
bool inScope = agreement.isContractInScope(contractAddress);

A protocol can satisfy all three booleans while pointing whitehats to an agreement that doesn't actually apply to the contract:

1. Deploy two agreements via the factory: Agreement A (generous terms, higher bounty, higher percentage) and Agreement B (stingier, low bounty, more strict requirements). Include the same contract addresses in both scopes.
2. Register stingier Agreement B with AttackRegistry (requestUnderAttack). This makes attackRegistry.isTopLevelContractUnderAttack(contract) return true and legally binds the contract to Agreement B.
3. Register favourable Agreement A with BattleChainSafeHarborRegistry (adoptSafeHarbor).

When whitehats fetch the agreement for that protocol via BattleChainSafeHarborRegistry.getAgreement, they receive Agreement A. When they then run the checklist using Agreement A:

• safeHarborRegistry.isAgreementValid(A) → true, because Agreement A was also factory-deployed.
• attackRegistry.isTopLevelContractUnderAttack(contract) → true, because Agreement B registered the contract.
• AgreementA.isContractInScope(contract) → true, because the early return in the constructor when adding contracts to the scope. While each contract can have only one agreement in the AttackRegistry, different agreements can include the same contract in their scope as long as they are not registered, due to no-op here.

All three checks succeed, yet the contract's actual governing agreement (Agreement B) has different bounty terms. Whitehats performing the recommended checks can still be misled into believing more generous terms apply and could even face legal consequences.

Note that the adoptSafeHarbor function includes the following comment:

/// @dev A user can add a malicious agreement here, this is ok
/// @dev but the attack registry can only use agreements deployed from the factory

This assumes that the added 'malicious' agreement is not deployed via the factory. However, protocols can deploy an agreement with malicious intent via the factory to pass checks and mislead whitehats.

Consider enforcing that adoptSafeHarbor only accepts agreements already registered in the AttackRegistry, if an onchain fix is desired. This would prevent inconsistent agreements across registries. However, mismatches may still occur after the commitment window if the protocol updates the AttackRegistry but not the BattleChainSafeHarborRegistry.

Additionally, document that whitehats should verify the agreement directly in the AttackRegistry via attackRegistry.getAgreementForContract(contractAddress) for each contract, and confirm the agreement state using attackRegistry.getAgreementState(agreementAddress)

Safe Harbor protection applies only to the accounts explicitly listed within an agreement’s scope (including any child contracts covered by ChildContractScope)

Complex exploits often traverse multiple contracts (e.g., entry points, routers, adapters, vaults). If a protocol’s agreement includes only a subset (for example, just the vault), any intermediate contract not listed is technically outside the protection. This can create grey areas for both whitehats and protocols. For example:

• A vault is in scope, but the exploit requires an entry point in another contract that is not in scope. In this case, isTopLevelContractUnderAttack returns true for the vault but false for the entry-point contract, making it unclear whether attacking the vault is permitted.
• The entry point of an attack is within scope, but the contract holding the funds is not.
• Both the entry-point contract and the vault are in scope, but multiple intermediary contracts involved in the exploit path are not.

Everything is clear for whitehats when all contracts in an exploit path are either entirely in scope or out of scope. However, mixed cases introduce uncertainty and should be documented for both whitehats and protocols, as protocols may assume that including only fund-holding contracts is sufficient and exclude surrounding contracts.

For protocols, document that all relevant contracts should be included in these agreements, rather than only those holding funds.

For whitehats, clarify which contracts must be verified as in scope, whether that includes only the final contract holding funds, the entry-point contract, or the entire exploit chain.

When _collectFeeAndBond runs, it checks for an existing bond deposit on the agreement and forfeits it if an unclaimed bond exists.

This is the intended behavior, as indicated by the BondForfeited event and the comment "Forfeit any existing unclaimed bond to keep s_reservedByToken accurate." However, this should be clearly documented, as agreement owners may not review the codebase directly.

Document the forfeiture behavior of unclaimed bonds. Alternatively, consider allowing previous depositors to claim at a later time, or directly transferring the existing bond to them.

After the minimum commitment window, the scope and agreement can be changed. The protocol may still be in the UNDER_ATTACK state while having passed the commitment window. During this period, a scope or agreement change may be recorded on-chain just before an attack transaction, either naturally or by the protocol front-running the attack with a scope change.

This may expose the whitehat to legal risk while performing the attack. To mitigate this, whitehats should perform scope and agreement validation and the attack atomically within the same transaction, ensuring there is no gap between validation and execution. This ensures that the attack either executes under a valid agreement or reverts if a scope change is mined before the attack transaction.

Document this risk and warn users to perform scope validation and attacks atomically.

registerContractForExistingAgreement / unregisterContractForExistingAgreement only pass if s_agreementFactory.isAgreementContract(msg.sender) is true. The Agreement contract is msg.sender.

Old agreements were created by factory A and are only marked in A’s s_isAgreement. If the registry owner changes to factory B via setAgreementFactory, those old addresses don’t exist in B’s mapping. The isAgreementContract check fails with InvalidAgreement revert.

Consequently, addAccounts, removeAccounts, addOrSetChains, removeChains (anything that syncs BattleChain scope) reverts for those agreements. Same for syncNewContracts when it hits the factory check on the agreement address.

Consider documenting this behavior, so that users are aware of it.

When an agreement auto-promotes to PRODUCTION (either via the 14-day deadline without DAO approval, or via the 3-day promotion delay), the transition is computed only — promoted stays false in storage. Because _revertIfLinkedToActiveAgreement reads the computed state, contracts can immediately be re-linked to a new agreement, emitting ContractRegistered(X, agreementB).

finalizeState is permissionless and has no expiry. If it is called after contracts have already been re-linked, it emits AgreementStateChanged(agreementA, PRODUCTION) out of order, after ContractRegistered(X, agreementB) has already appeared in the event stream.

Any off-chain system reconstructing "which agreement governed contract X at production time" will compute the wrong answer: it will appear that contract X was still under agreementA's umbrella when it graduated, when in reality X had already been reassigned to agreementB.

Consider requiring finalizeState to be called before contracts can be re-linked, or calling finalizeState in _revertIfLinkedToActiveAgreement.

Agreement.addAccounts does not check for duplicate BattleChain account addresses (documented in NatSpec). The scope cache deduplicates via s_battleChainScopeExists in _addToBattleChainScope. When the same address is added twice to s_accounts but only once to the scope cache, a single removeAccounts call removes the cache entry entirely while one copy remains in s_accounts. The result is a state where getChainAccounts still includes the address but getBattleChainScopeAddresses and isContractInScope do not. Consequently, off-chain tools relying on getDetails show the contract in scope while on-chain queries show it out of scope.

Consider rejecting duplicate BattleChain additions, or document this behavior, as adding the same address twice constitutes an owner error.

When s_bondToken != address(0) but s_verifiedBondAmount = 0 (or s_unverifiedBondAmount = 0), BondManager._collectFeeAndBond creates a BondDeposit with bondAmount = 0. The depositor pays only a fee. When the agreement later reaches PRODUCTION, the depositor calls claimBond. The lazy-mark block is gated on deposit.bondAmount > 0 and is skipped. _markBondClaimable also returns early for bondAmount == 0. So bondClaimable is never set to true, and claimBond permanently reverts with BondNotYetClaimable.

While claiming a bond with a zero amount is unnecessary and has no impact on funds, off-chain automation tools that call claimBond after going into production will fail.

Consider handling zero-bond deposits as an early success and marking them as claimable within the lazy-marking block, thereby allowing zero-bond claims when necessary.

The comment in the AttackRegistry contract states that "NEW_DEPLOYMENT is for contracts deployed via BattleChainDeployer, and NOT_DEPLOYED is for external deployments." However, this is incorrect, as the NEW_DEPLOYMENT state is not tied to BattleChainDeployer.

_getAgreementState returns NEW_DEPLOYMENT only when info.isRegistered == true and attackRequested == false; however, all registration paths set attackRequested to true.

All unregistered agreements are treated as NOT_DEPLOYED, regardless of whether they are externally deployed or deployed via BattleChainDeployer.

Update the outdated comment and the README to reflect the current behavior.

promotionRequestedTimestamp is now cleared on explicit terminal paths, so the original L-01 issue has been fixed. Multiple comments in the codebase now state that a non-zero timestamp indicates a promotion is pending.

• In known-issues.md: "preserving the invariant that a non-zero timestamp means promotion is currently pending"
• In AttackRegistry.sol: "// Maintain invariant: promotionRequestedTimestamp != 0 iff promotion currently pending"

However, this is not entirely accurate, since promotionRequestedTimestamp can remain non-zero while the state is PRODUCTION after PROMOTION_DELAY has elapsed but before finalizeState has been called.

Update the comments to reflect that a non-zero promotionRequestedTimestamp does not necessarily mean the promotion is still pending, and may also represent the period where the state has effectively transitioned to PRODUCTION but finalizeState has not yet been called.

The I-10 fix added a short-circuit at the top of claimBond so fee-only (zero-bond) deposits no longer permanently revert. As written, the short-circuit runs before the lazy-mark / PRODUCTION check that gates non-zero claims:

if (deposit.bondAmount == 0) {
    emit BondClaimed(agreementAddress, deposit.depositor, 0);
    deposit.claimed = true;
    return;
}

// Lazy-mark for time-based promotions that bypassed explicit marking
if (!deposit.bondClaimable) {
    if (_getAgreementState(agreementAddress) == IAttackRegistry.ContractState.PRODUCTION) {
        emit BondClaimable(agreementAddress, deposit.bondAmount);
        deposit.bondClaimable = true;
    }
}

if (!deposit.bondClaimable) {
    revert BondManager__BondNotYetClaimable(agreementAddress);
}

For non-zero bonds the depositor must wait until PRODUCTION (or until the agreement is rejected / corrupted) before claimBond succeeds. For zero-bond deposits there is no such gate. The depositor can call claimBond immediately after _collectFeeAndBond, while the agreement is still in ATTACK_REQUESTED or UNDER_ATTACK, and the deposit gets flipped to claimed = true.

Move the short-circuit inside the existing PRODUCTION lazy-mark gate, matching the I-10 recommendation.