Potentially unnecessary approval revocation in GuardedExecutor

[legion2002]

Problem Statement

The current implementation in GuardedExecutor.sol revokes all non-zero approvals after batch execution (lines 313-325), even when the full approval amount has already been counted against spend limits.

Current behavior:

1. When approve(spender, 100) is called, the system adds the full 100 to transferAmounts
2. This 100 gets counted against the spend limit via _incrementSpent()
3. If only 40 tokens are actually transferred, the remaining 60 approval is revoked

Question: Since we already count the full approval amount (100) as spent against the limit, is revoking the leftover approval (60) necessary?

Technical Analysis

How approvals are currently handled:

// Lines 275-281: approve(address,uint256) detection
if (fnSel == 0x095ea7b3) {
    if (LibBytes.loadCalldata(data, 0x24) == 0) continue; // amount == 0
    t.approvedERC20s.p(target);
    t.approvalSpenders.p(LibBytes.loadCalldata(data, 0x04).lsbToAddress()); // spender
    t.erc20s.p(target); // token  
    t.transferAmounts.p(LibBytes.loadCalldata(data, 0x24)); // amount - FULL APPROVAL
}

The system tracks the full approval amount in transferAmounts, not the actual usage.

Spend limit enforcement:

// Lines 340-345: Conservative spending calculation
Math.max(
    t.transferAmounts.get(i),  // Full approval amount (100)
    Math.saturatingSub(balancesBefore.get(i), currentBalance) // Actual transfer (40)
)

The system uses the maximum of calldata amounts vs actual balance changes, ensuring full approval amounts are counted against limits.

Security Analysis

The Cross-Period Attack Vector

The approval revocation is security-critical for time-based spend periods but potentially unnecessary for Forever periods.

Attack scenario for time-based periods (Day, Week, etc.):

1. Day 1: User approves 100 tokens (counted against daily limit)
2. Day 1: Spender uses only 40 tokens
3. Day 2: New period starts, spend counter resets to 0 (line 652: tokenPeriodSpend.spent = 0)
4. Day 2: Spender uses remaining 60 from old approval (would count as new spending)
5. Day 2: User could approve additional 100 tokens
6. Result: 160 tokens spent in Day 2 with only 100 limit

Code reference:

// Lines 650-653: Period reset logic
if (tokenPeriodSpend.lastUpdated < current) {
    tokenPeriodSpend.lastUpdated = current;
    tokenPeriodSpend.spent = 0;  // ⚠️ Counter resets each period
}

Forever Periods Exception

For SpendPeriod.Forever, the attack doesn't work:

// Line 606: Forever period never resets  
if (period == SpendPeriod.Forever) return 1;

Since startOfSpendPeriod always returns 1 for Forever periods, the condition tokenPeriodSpend.lastUpdated < current becomes false after the first update, preventing spend counter resets.

Potential Solutions

Option 1: Period-aware revocation (Recommended)

// Only revoke approvals for accounts with time-based spend periods
bool hasTimeBased = false;
for (uint256 j; j < periods.length; ++j) {
    if (SpendPeriod(periods[j]) != SpendPeriod.Forever) {
        hasTimeBased = true;
        break;
    }
}
if (hasTimeBased) {
    // Revoke approvals
}

Option 2: Keep current behavior (Conservative)

Maintain existing revocation for all periods to ensure no edge cases are missed.

Option 3: Document the tradeoff

If the gas savings aren't significant, keep current behavior but document why it's necessary.

Questions for Review

1. Are there any Forever-period-only accounts where gas optimization would be valuable?
2. Could there be other edge cases involving mixed time-based and Forever periods?
3. Is the added complexity of period-aware revocation worth the gas savings?

The current implementation errs on the side of security, which may be the right approach given the comment: "There is no strict definition on what constitutes spending, and we want to be as conservative as possible" (lines 338-339).

Code References

• Approval detection: GuardedExecutor.sol:275-281
• Approval revocation: GuardedExecutor.sol:313-325
• Spend increment logic: GuardedExecutor.sol:641-659
• Period reset: GuardedExecutor.sol:650-653