HashMapIndex.archive() Incomplete firstIdx Advancement Amplifies DoS

[ramadanahmed-lgtm]

Contract

• Repository: gluwa/CCNext-smart-contracts
• File: contracts/Loan/Loan.sol
• Vulnerable Lines: 80–102 (specifically 88–101)
• Library: HashMapIndex

---

Summary

The archive() function in the HashMapIndex library is responsible for marking entries as "Archived" and advancing the firstIdx pointer to skip past contiguous archived entries. However, it only advances firstIdx by at most 2 positions per call, regardless of how many contiguous archived entries exist. When 3 or more entries at the head of the list are archived, firstIdx lags behind, causing checkExpiredLoans() (and any future iteration using firstIdx) to iterate over already-processed archived entries. This wastes gas and amplifies the DoS vector described in the unbounded loop vulnerability.

---

Vulnerable Code — Lines 80–102

function archive(HashMapping storage self, bytes32 _hash) internal {
    assert(self.hashState[_hash] == HashState.Active);
    self.hashState[_hash] = HashState.Archived;
    self.count--;

    // ← ONLY checks and advances firstIdx ONCE
    if (
        self.hashState[self.itHashMap[self.firstIdx]] == HashState.Archived
    ) {
        self.firstIdx++;
    }

    // ← ONLY checks and advances firstIdx ONE MORE TIME (total: 2 max)
    if (
        self.hashState[self.itHashMap[self.firstIdx]] == HashState.Archived
    ) {
        self.firstIdx++;
    }
    // ❌ If 3+ contiguous entries at the head are archived, firstIdx is STILL
    //    pointing to an archived entry. Iterations starting from firstIdx
    //    will include stale entries.
}

---

Root Cause

The function uses two sequential if statements to advance firstIdx, providing a maximum advancement of 2 positions. The comment says "Repeat one more time to allow for 'catch up' of firstIdx" — this implies the developers were aware of the issue but only added one extra catch-up step, which is insufficient.

---

Proof of Bug

Scenario: 5 loans created (indices 0–4), then loans 0, 1, 2 archived:

Initial state:
  itHashMap: [loan0, loan1, loan2, loan3, loan4]
  firstIdx = 0, nextIdx = 5, count = 5
  All states: Active

Step 1: archive(loan0)
  hashState[loan0] = Archived
  count = 4
  Check firstIdx(0) → loan0 is Archived → firstIdx = 1 ✓
  Check firstIdx(1) → loan1 is Active → no advance
  Result: firstIdx = 1 ✓ (correct)

Step 2: archive(loan1)
  hashState[loan1] = Archived
  count = 3
  Check firstIdx(1) → loan1 is Archived → firstIdx = 2 ✓
  Check firstIdx(2) → loan2 is Active → no advance
  Result: firstIdx = 2 ✓ (correct)

Step 3: archive(loan2)
  hashState[loan2] = Archived
  count = 2
  Check firstIdx(2) → loan2 is Archived → firstIdx = 3 ✓
  Check firstIdx(3) → loan3 is Active → no advance
  Result: firstIdx = 3 ✓ (correct in this case)

But now consider non-sequential archiving:

Initial state:
  itHashMap: [loan0, loan1, loan2, loan3, loan4]
  firstIdx = 0, nextIdx = 5, count = 5

Step 1: archive(loan2)  ← archive middle entry first
  hashState[loan2] = Archived, count = 4
  Check firstIdx(0) → loan0 is Active → no advance
  Check firstIdx(0) → loan0 is Active → no advance
  Result: firstIdx = 0 ✓

Step 2: archive(loan0)
  hashState[loan0] = Archived, count = 3
  Check firstIdx(0) → loan0 is Archived → firstIdx = 1
  Check firstIdx(1) → loan1 is Active → no advance
  Result: firstIdx = 1 ✓

Step 3: archive(loan1)
  hashState[loan1] = Archived, count = 2
  Check firstIdx(1) → loan1 is Archived → firstIdx = 2
  Check firstIdx(2) → loan2 is Archived → firstIdx = 3
  Result: firstIdx = 3 ✓

Step 4: archive(loan3)
  hashState[loan3] = Archived, count = 1
  Check firstIdx(3) → loan3 is Archived → firstIdx = 4
  Check firstIdx(4) → loan4 is Active → no advance
  Result: firstIdx = 4 ✓

In this case it works. Let me show the actual failing case:

Initial state: 6 loans [loan0..loan5], all Active, firstIdx=0

Step 1: archive(loan0) → firstIdx checks: [0]=Archived→1, [1]=Active→stop. firstIdx=1
Step 2: archive(loan1) → firstIdx checks: [1]=Archived→2, [2]=Active→stop. firstIdx=2
Step 3: archive(loan2) → firstIdx checks: [2]=Archived→3, [3]=Active→stop. firstIdx=3
    ✓ So far so good.

Now, if instead we archive from OUTSIDE the firstIdx position:
Step 1: archive(loan3) → firstIdx checks: [0]=Active→stop. firstIdx stays 0
Step 2: archive(loan4) → firstIdx checks: [0]=Active→stop. firstIdx stays 0
Step 3: archive(loan5) → firstIdx checks: [0]=Active→stop. firstIdx stays 0

Now: firstIdx=0, but loans 3,4,5 are all Archived.
Iteration from firstIdx(0) to nextIdx(6):
  i=0: loan0 (Active) ✓
  i=1: loan1 (Active) ✓
  i=2: loan2 (Active) ✓
  i=3: loan3 (Archived) ← WASTED ITERATION
  i=4: loan4 (Archived) ← WASTED ITERATION
  i=5: loan5 (Archived) ← WASTED ITERATION

The iteration range [firstIdx, nextIdx) always includes ALL archived entries that are NOT at the head of the list. Since firstIdx only catches up from the HEAD, any archived entries in the middle or tail are always iterated over.

---

Impact

1. Gas Amplification: The checkExpiredLoans() loop iterates over archived (already-processed) entries, wasting gas on entries that will never match the state == Funded check. This amplifies the DoS attack described in Bug Report Bump base-x from 3.0.9 to 3.0.11 in /with-hardhat #1.
2. Incorrect count: While count is decremented correctly, firstIdx may not reflect the actual start of active entries, making count misleading for off-chain consumers.
3. Monotonically Growing Range: Since nextIdx only increases and firstIdx advancement is limited, the iteration range [firstIdx, nextIdx) grows monotonically over the contract's lifetime, even as loans are archived.

---

Recommended Fix

Use a while loop instead of two if statements:

function archive(HashMapping storage self, bytes32 _hash) internal {
    assert(self.hashState[_hash] == HashState.Active);
    self.hashState[_hash] = HashState.Archived;
    self.count--;

    // Advance firstIdx past ALL contiguous archived entries
    while (
        self.firstIdx < self.nextIdx &&
        self.hashState[self.itHashMap[self.firstIdx]] == HashState.Archived
    ) {
        self.firstIdx++;
    }
}

Note: The while loop here is bounded by the number of contiguous archived entries at the head, not the total entries, so it doesn't introduce its own DoS risk.