Audit Anomalies Archive — Issue#8

Before delving into the issue at hand, I recommend familiarizing yourself with Issue#7 . Understanding the context is important for understanding the problem I am about to address.

Is ReentrancyGuard alone sufficient?

Having audited over 25+ projects, I’ve observed a common practice among developers, utilizing ReentrancyGuard for functions involving external operations. The belief is that this would block hackers from exploiting reentrancy vulnerabilities in the contract. However, this assumption is not foolproof. While employing ReentrancyGuard can prevent recursive entry into a specific function, it does not guarantee overall protection. If a hacker gains access to a different function and the code neglects the Checks-Effects-Interactions (CEI) pattern, it opens the door to potential exploits.

More info about the bridge protocol

For a clearer understanding, let’s explore the bridge protocol in more detail. I’ve simplified the code, focusing on three key functions:

1. depositERC20: Allows users to deposit $TEST ERC20 tokens on the mainnet, later withdrawable from the TEST Chain.
2. withdrawNative: Users can call this function with the signature of the ERC20 token deposit to withdraw native $TEST tokens on the TEST Chain.
3. renounceClaim: If a depositor decides to cancel a request, this function can be called. The signature is burned, an event is emitted, and the user can then withdraw funds on the source chain (mainnet).

Aside from these functions, some aspects of the code, not included in the demo but are worth mentioning. Once tokens are deposited on the bridge, a trusted user with the SIGNER_ROLE signs the signature, which is verified during withdrawal. Events emitted by the three functions are monitored 24/7, updating the depositor’s UI accordingly.

The Bug

In examining the code for depositERC20 and withdrawNative, although they implement a nonReentrant modifier, they fail to adhere to the CEI pattern. In withdrawNative, the signature is burned after the external call, creating an opportunity for an attacker to exploit.

function withdrawNative(address sender, uint256 amount, uint256 nonce, 
bytes memory signature)
external
nonReentrant
{
  // Transfer native tokens
  (bool success,) = payable(_msgSender()).call{value: amount}("");
  if (!success) revert InvalidTransfer();
  
  // Decrease native token balance
  nativeTokenBalance -= amount;
  
  // Increment withdrawals counter
  withdrawalsCounter++;
  
  // Mark the signature as used
  signatureUsed[messageHash] = true;
}

function renounceClaim(
    address destinationNetworkToken,
    address sender,
    uint256 amount,
    uint256 nonce,
    bytes memory signature
) public {

    if (signatureUsed[messageHash]) revert HashAlreadyUsed();
    //Rest of the code
}

The attacker can patiently wait for the external call, craft a custom fallback function invoking renounceClaim. At this point, the attacker, having initiated the withdrawal, finds success since the messageHash is not yet marked as used. The renounceClaim call is accepted, an event is emitted, and the code, misinterpreting the situation, allows the withdrawal to proceed on source chain, resulting in lose of user and protocol funds via cross-function re-entrancy.

What is the Fix ?

Despite the presence of ReentrancyGuards, always adhere to the Checks-Effects-Interactions pattern to fortify the code against such vulnerabilities.

For a practical demonstration of this issue via Foundry, additional details can be found in this link.

Thanks for Reading !

Connect with me: https://linktr.ee/zuhaib44