[M-1] Incorrect Rescue Delay Configuration in TestEscrowFactory Constructor

[Niraj-Kamdar]

[M-1] Incorrect Rescue Delay Configuration in TestEscrowFactory Constructor

Impact: Medium | Likelihood: High

Description

The TestEscrowFactory constructor contains a parameter confusion bug where the destination rescue delay (rescueDelayDst) is incorrectly passed twice to the parent EscrowFactory constructor, overriding the intended source rescue delay (rescueDelaySrc). This results in both source and destination escrows using the same rescue delay timing, potentially breaking the intended security model.

Code Location

File: TestEscrowFactory.sol: L-12

contract TestEscrowFactory is EscrowFactory {
    constructor(
        address limitOrderProtocol,
        IERC20 feeToken,
        IERC20 accessToken,
        address owner, 
        uint32 rescueDelaySrc,
        uint32 rescueDelayDst
    ) EscrowFactory(limitOrderProtocol, feeToken, accessToken, owner, rescueDelayDst, rescueDelayDst) {}
    //                                                                    ^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^
    //                                                                    Should be:       Correct
    //                                                                    rescueDelaySrc   rescueDelayDst
}

Expected vs Actual Behavior

Expected Constructor Call:

EscrowFactory(limitOrderProtocol, feeToken, accessToken, owner, rescueDelaySrc, rescueDelayDst)

Actual Constructor Call:

EscrowFactory(limitOrderProtocol, feeToken, accessToken, owner, rescueDelayDst, rescueDelayDst)

Impact Analysis

Security Implications:

1. Timing Mismatch: Source chain escrows will use destination chain rescue delay instead of their intended delay
2. Cross-Chain Inconsistency: Breaks the design assumption that different chains may require different rescue timings
3. Fund Recovery Risk: Depending on the delay values:
   • If rescueDelayDst < rescueDelaySrc: Premature rescue operations possible on source chain
   • If rescueDelayDst > rescueDelaySrc: Delayed fund recovery on source chain, potentially trapping funds longer than intended

Operational Impact:

• Inconsistent rescue mechanism behavior across chains
• Potential user fund accessibility issues during emergency scenarios
• Violation of intended security model design

Attack Scenario

// Deployment scenario where this matters:
TestEscrowFactory factory = new TestEscrowFactory(
    limitOrderProtocol,
    feeToken,
    accessToken,
    owner,
    30 minutes,  // Intended source rescue delay (faster for Ethereum)
    2 hours      // Destination rescue delay (slower for L2)
);

// Result: Source escrows will have 2-hour rescue delay instead of 30 minutes
// This could trap funds for 1.5 hours longer than intended during emergencies

Recommended Fix

Immediate Fix:

contract TestEscrowFactory is EscrowFactory {
    constructor(
        address limitOrderProtocol,
        IERC20 feeToken,
        IERC20 accessToken,
        address owner, 
        uint32 rescueDelaySrc,
        uint32 rescueDelayDst
    ) EscrowFactory(limitOrderProtocol, feeToken, accessToken, owner, rescueDelaySrc, rescueDelayDst) {}
    //                                                                    ^^^^^^^^^^^^^^^
    //                                                                    Fixed: use rescueDelaySrc
}

Testing Validation

// Test to verify fix
function testRescueDelayConfiguration() public {
    TestEscrowFactory factory = new TestEscrowFactory(
        limitOrderProtocol,
        feeToken,
        accessToken,
        owner,
        30 minutes,  // rescueDelaySrc
        2 hours      // rescueDelayDst
    );
    
    EscrowSrc srcEscrow = EscrowSrc(factory.ESCROW_SRC_IMPLEMENTATION());
    EscrowDst dstEscrow = EscrowDst(factory.ESCROW_DST_IMPLEMENTATION());
    
    assertEq(srcEscrow.rescueDelay(), 30 minutes, "Source rescue delay incorrect");
    assertEq(dstEscrow.rescueDelay(), 2 hours, "Destination rescue delay incorrect");
}

Classification Summary

• Severity: Medium (affects fund recovery timing but doesn't directly enable theft)
• Likelihood: High (occurs on every deployment of TestEscrowFactory)
• Fix Complexity: Low (single parameter correction)
• Detection: Easy (unit tests would catch this immediately)