Potential Reentrancy Vulnerability in the `pair::swap`

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xa92a0ebac7b2f896ac739110747534dc34f999e2cc66530fc4a0465d71191761 Severity: high

Description: Description A potential reentrancy issue has been identified in the pair::swap function within a Rust-based smart contract. This issue stems from a callback to the caller within the pair::swap function, which potentially allows for reentrant calls.

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/b097173adc9966bcbed72c6a4f1b50fcc52fe0ef/amm/contracts/pair/lib.rs#L441-L445

In contrast to the Solidity implementation of similar contracts, where a lock modifier is used to prevent reentrancy, the Rust implementation lacks such preventative measures. This absence leaves the function vulnerable to reentrancy attacks.

function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external lock {

Impact As there is no check effect interaction let's see what important state changes, change after the call:

in swap function self.update(balance_0, balance_1, reserves.0, reserves.1)?;, called. that will change reserve 0 and reserve 1.

so basically each function that uses reserve 0 and reserve 1 can be manipulated.

Affected Functions

The vulnerability potentially impacts multiple functions that rely on the reserve values (reserve 0 and reserve 1). These include:

• skim
• sync
• burn
• mint
• swap

Each of these functions, relying on the integrity of the reserve values, can be manipulated following a reentrant call to swap.

Attachments

1. Proof of Concept (PoC) File: Due to the clear nature of the reentrancy issue, a specific PoC demonstrating reentry through the swap function is not provided. The vulnerability is evident from the code structure and the absence of reentrancy guards. if there is any need for POC in code let me know.

2. Revised Code File (Optional): It is recommended to consider the integration of a non-reentrancy lock (similar to the Solidity lock modifier) in a pair contract. This modification would significantly mitigate the risk of reentrancy attacks and align the Rust implementation with best practices observed in Solidity contracts.

[coreggon11]

In ink! reentrancy is disabled by default. So, the cross-contract call must be explicitly called with the allow-reentrancy flag set to true, which is not the case here. See the inline docs of ink! here.

[0xmahdirostami]

In ink! reentrancy is disabled by default. So, the cross-contract call must be explicitly called with the allow-reentrancy flag set to true, which is not the case here. See the inline docs of ink! here.

Thanks

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

We hope you participate in the future audits of ink!.