Skip to content

[contracts] no test for mid-batch item failure; batchExecute atomicity undocumented in NatSpec #212

@obchain

Description

@obchain

PR: #45 (feat/20-multi-liq-batcher)
File: contracts/test/CharonLiquidator.t.sol section F, contracts/src/CharonLiquidator.sol batchExecute NatSpec

The four new tests cover: non-owner access, empty array, over-limit array, and item[0] validation failure before any flashloan fires. No test exercises failure at item[k] where k > 0 after item[k-1] has passed validation.

EVM transaction atomicity guarantees that if any call in a transaction reverts, all prior state changes in that transaction are also reverted. This means batchExecute is fully atomic: if item[5] fails, items 0-4 are also rolled back. However:

  1. This guarantee is nowhere documented in the contract NatSpec or test comments. A consumer of the contract cannot confirm atomicity from reading the code or tests.
  2. The BatchExecuted(uint256 count) event NatSpec does not state that it is emitted only on full-batch success and that any partial revert also reverts prior items.
  3. Without a test for k > 0 failure, the atomic-revert behavior for post-first-item failures is implicit and unverified at the test layer.

Impact: Operators reading the contract may assume partial fills are possible (N items attempted, M succeed), leading to incorrect profit accounting or retry logic in off-chain code.

Fix:

  1. Add NatSpec to batchExecute: 'Execution is EVM-atomic. If any item reverts, all prior items in the same batch are also reverted. BatchExecuted is emitted only on full-batch success.'
  2. Add a test test_batchExecute_revertsOnSecondItemValidation: 2-item batch, item[0] valid with STUB_POOL.flashLoanSimple mocked as no-op, item[1].borrower = address(0). Assert revert with '!borrower'. Assert BatchExecuted was NOT emitted.
  3. Add NatSpec to BatchExecuted: 'Emitted after all N items have completed successfully. Not emitted on partial or full revert.'

Refs #45

Metadata

Metadata

Assignees

No one assigned

    Labels

    layer:contractsSolidity / Foundrypr-reviewFindings from PR review processpriority:p2-polishNice-to-have / polishstatus:readyScoped and ready to pick uptype:testTests, fuzz, fork, integration

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions