Checks-Effects-Interactions pattern on `AbstractL1BTCDepositor`

[jose-blockchain]

Reentrancy Protection Gap in Abstract Method Pattern

Impact: Developers may inadvertently introduce reentrancy vulnerabilities in child implementations

NOTE: Transferring tBTC (ERC20) last can indeed reduce the risk of a large financial loss if a reentrancy vulnerability is present, because the tBTC transfer is typically the most valuable operation in the function. Transferring tBTC last is the most conservative and secure approach, especially if the reimbursement involves a low-level call to an untrusted or upgradable contract. This minimizes the risk of repeated tBTC transfers in the event of a reentrancy exploit.

function finalizeDeposit(uint256 depositKey) external payable {
    // Abstract method called before state cleanup
    _transferTbtc(tbtcAmount, destinationChainDepositOwner);
    
    // State cleanup happens AFTER external interaction
    if (reimbursement.receiver != address(0)) {
        delete gasReimbursements[depositKey];
        reimbursementPool.refund(...)
    }
}

Issue: The pattern violates Checks-Effects-Interactions (CEI) by calling the abstract method before state cleanup.

Recommendation:

• Move state cleanup before _transferTbtc() call
• Add reentrancy guard to finalizeDeposit()
• Implement CEI (Checks-Effects-Interactions) pattern as template for developers

[jose-blockchain]

• Transferring tBTC (ERC20) last can indeed reduce the risk of a large financial loss if a reentrancy vulnerability is present, because the tBTC transfer is typically the most valuable operation in the function.
• Transferring tBTC last is the most conservative and secure approach, especially if the reimbursement involves a low-level call to an untrusted or upgradable contract.
• This minimizes the risk of repeated tBTC transfers in the event of a reentrancy.