Skip to content

fix(security): restore checks-effects-interactions and measure actual received tokens across all deposit paths - #17

Open
mertcano wants to merge 1 commit into
circlefin:masterfrom
mertcano:mertcano-patch-1
Open

mertcano wants to merge 1 commit into
circlefin:masterfrom
mertcano:mertcano-patch-1

Conversation

@mertcano

@mertcano mertcano commented Oct 1, 2026

Copy link
Copy Markdown

Summary of Changes

Addresses the High severity security audit finding in evm-gateway-contracts (Cat 12: Concurrency & transactional integrity) by enforcing Checks-Effects-Interactions (CEI) and validating received token amounts across all deposit vectors in Deposits.sol[cite: 31].


Vulnerability Analysis

  • In src/modules/wallet/Deposits.sol, the internal functions _depositWithApproval, _depositWithPermit, and _depositWithAuthorization previously updated internal state (_increaseAvailableBalance) prior to executing the external token transfer[cite: 31].
  • For deflationary, fee-on-transfer, or non-standard ERC-20 tokens, the depositor was credited with the nominal requested amount while the contract received less[cite: 31]. The unbacked shortfall became redeemable from other users' deposits during subsequent withdrawals[cite: 31].
  • The effects-before-interactions pattern also permitted reentrancy via ERC-777 or ERC-1363 token callbacks, enabling callers to observe an inflated balance before funds were safely transferred[cite: 31].
  • In the initial remediation draft, _depositWithAuthorization sampled balanceBefore after invoking receiveWithAuthorization, calculating received = 0 and causing all authorization-based deposits to revert with UnsupportedTokenTransferFee.

Key Remediations

  1. Checks-Effects-Interactions Enforcement:
    • Reordered operations across _depositWithApproval, _depositWithPermit, and _depositWithAuthorization: 1. Pre-transfer balance snapshot (balanceBefore = balanceOf(address(this))). 2. Execute external transfer (safeTransferFrom or receiveWithAuthorization)[cite: 31, 50]. 3. Delta measurement (received = balanceOf(address(this)) - balanceBefore)[cite: 50]. 4. Enforce strict parity: revert with UnsupportedTokenTransferFee() if received != value[cite: 31, 50]. 5. State update (_increaseAvailableBalance) and event emission (emit Deposited)[cite: 50, 51].
  2. Timing Correction in _depositWithAuthorization:
    • Moved the balanceBefore capture ahead of receiveWithAuthorization to guarantee proper balance accounting and eliminate the false-positive fee revert.

Verification

  • Compiled using solc 0.8.29 via standard-JSON compiler driver: 0 errors, 0 warnings across the 24-file import closure.
  • Conforms to OpenZeppelin SafeERC20 best practices[cite: 62].

… received tokens across all deposit paths

### Summary of Changes
Addresses the **High** severity security audit finding in `evm-gateway-contracts` (Cat 12: Concurrency & transactional integrity) by enforcing Checks-Effects-Interactions (CEI) and validating received token amounts across all deposit vectors in `Deposits.sol`[cite: 31].

---

### Vulnerability Analysis
- In `src/modules/wallet/Deposits.sol`, the internal functions `_depositWithApproval`, `_depositWithPermit`, and `_depositWithAuthorization` previously updated internal state (`_increaseAvailableBalance`) **prior** to executing the external token transfer[cite: 31].
- For deflationary, fee-on-transfer, or non-standard ERC-20 tokens, the depositor was credited with the nominal requested amount while the contract received less[cite: 31]. The unbacked shortfall became redeemable from other users' deposits during subsequent withdrawals[cite: 31].
- The effects-before-interactions pattern also permitted reentrancy via ERC-777 or ERC-1363 token callbacks, enabling callers to observe an inflated balance before funds were safely transferred[cite: 31].
- In the initial remediation draft, `_depositWithAuthorization` sampled `balanceBefore` **after** invoking `receiveWithAuthorization`, calculating `received = 0` and causing all authorization-based deposits to revert with `UnsupportedTokenTransferFee`.

---

### Key Remediations
1. **Checks-Effects-Interactions Enforcement:**
   - Reordered operations across `_depositWithApproval`, `_depositWithPermit`, and `_depositWithAuthorization`:
     1. Pre-transfer balance snapshot (`balanceBefore = balanceOf(address(this))`).
     2. Execute external transfer (`safeTransferFrom` or `receiveWithAuthorization`)[cite: 31, 50].
     3. Delta measurement (`received = balanceOf(address(this)) - balanceBefore`)[cite: 50].
     4. Enforce strict parity: revert with `UnsupportedTokenTransferFee()` if `received != value`[cite: 31, 50].
     5. State update (`_increaseAvailableBalance`) and event emission (`emit Deposited`)[cite: 50, 51].
2. **Timing Correction in `_depositWithAuthorization`:**
   - Moved the `balanceBefore` capture ahead of `receiveWithAuthorization` to guarantee proper balance accounting and eliminate the false-positive fee revert.

---

### Verification
- Compiled using `solc 0.8.29` via standard-JSON compiler driver: **0 errors, 0 warnings** across the 24-file import closure.
- Conforms to OpenZeppelin `SafeERC20` best practices[cite: 62].
@mertcano
mertcano requested a review from a team as a code owner October 1, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant