Zellic - Audit Reports

Category: Coding Mistakes

Reentrancy issue allowing repeat withdrawals

Critical Severity

Critical Impact

High Likelihood

Description

The function close_withdrawal has a reentrancy issue. The processedL2Requests mapping is only updated at the end of the function and after the withdrawal is processed, which includes sending ETH to the recipient.

function close_withdrawal(Withdrawal calldata withdrawal, bytes32 merkle_root, bytes32[] calldata proof) public {
    Range memory r = merkleRootRange[merkle_root];
    require(r.start != 0 && r.end != 0, "Unknown merkle root"); 

    bytes32 withdrawal_hash = keccak256(abi.encode(withdrawal));
    require(processedL2Requests[withdrawal.requestId.id] == false, "Already processed");

    uint32 leaves_count = uint32(r.end - r.start + 1);
    uint32 pos = uint32(withdrawal.requestId.id - r.start); // AUDIT: unsafe narrowing cast
    require(
      calculate_root(withdrawal_hash, pos, proof, leaves_count) == merkle_root,
      "Invalid proof"
    );
    process_l2_update_withdrawal(withdrawal);
    processedL2Requests[withdrawal.requestId.id] = true;
}

Impact

The attacker could repeat the withdrawal and drain the assets by abusing the reentrancy vulnerability.

Recommendations

Implement a reentrancy guard on all public and external functions. We recommend implementing reentrancy guards by default, unless a contract explicitly needs reentrancy.

We also recommend to adopt the checks-effects-interactions pattern as a best practice.

Remediation

This issue has been acknowledged by Gasp, and a fix was implemented in commit 05b0ca86↗.