Skip to content

BridgeTransferLib: records() lookup uses refId instead of wdId in receivePegV2Withdraw #319

@alexchen-security

Description

@alexchen-security

While reviewing the BridgeTransferLib.sol library, I noticed a potential logic inconsistency in receivePegV2Withdraw():

if (IOriginalTokenVaultV2(_bridgeAddr).records(request.refId)) {
    // skip withdraw, compute transferId locally
} else {
    recv.transferId = IOriginalTokenVaultV2(_bridgeAddr).withdraw(...)
}

The records() mapping in OriginalTokenVaultV2 is indexed by wdId (the withdrawal ID computed from the withdrawal parameters), not by refId. This means the condition records(request.refId) would only return true if refId happens to collide with a previously computed wdId — which is effectively a hash collision and practically impossible.

Impact Assessment:

This appears to be a non-exploitable logic bug. The condition effectively always evaluates to false, meaning withdrawals always go through the else branch and call withdraw() directly. The "skip" path is dead code.

However, this means:

  1. The deduplication check for pegged withdrawals via the vault is not functioning as intended
  2. If a relay message were somehow replayed (e.g., via a compromised quorum), the records() check would not prevent a duplicate withdrawal through this code path

Note that withdraw() itself has its own records[wdId] check internally, so actual double-withdrawal is still prevented at the vault level. This is a defense-in-depth concern rather than an exploitable vulnerability.

Suggested Fix:

Compute the expected wdId from the request parameters before the check:

bytes32 expectedWdId = keccak256(abi.encodePacked(
    request.receiver, request.token, request.amount, 
    request.burnAccount, request.refChainId, request.refId, address(_bridgeAddr)
));
if (IOriginalTokenVaultV2(_bridgeAddr).records(expectedWdId)) {
    // properly skip already-processed withdrawal
} else {
    recv.transferId = IOriginalTokenVaultV2(_bridgeAddr).withdraw(...)
}

Happy to submit a PR with the fix if the team agrees with the analysis. Found this while doing a security review of state channel / cross-chain bridge implementations.

Severity: Low (non-exploitable, defense-in-depth)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions