Background

Team Secure3 recently discovered a malicious attack on the OMNI Protocol, with the hacker stealing almost 2000 ETH (~2.4M USD) from OMNI.

According to the OMNI Protocol, only internal testing funds were affected, no customer funds were lost.

Team Secure3 independently analyzed the logic loopholes behind the hack, and now publicly shares our analysis and conclusions to the community in this post.

Accountability

According to the open-source code repository from OMNI Protocol on 07/06/2022 (4 days before the hack), function executeERC721LiquidationCallin the contract LiquidationLogicdoes not follow the **Checks-Effects-Interactions** pattern.

Such a critical security issue brings **Reentrancy** risk, making it possible for hackers to claim back the collateralized NFT assets without paying back the loan borrowed from OMNI.

Secure3 team carefully reviewed the public commit history of OMNI Protocol repository:

As well as the auditing reports published on 07/05/2022,

We noticed that the loophole contracts were within the scope of smart contract security auditing, yet this issue was not reported by corresponding auditing parties.

Hack Analysis

Some notable addresses:

• Malicious attacker address: 0x627a22FF70Cb84e74c9C70E2d5B0B75af5A1Dcb9
• Hash of one of the malicious attack transactions: 0x05d65e0adddc5d9ccfe6cd65be4a7899ebcb6e5ec7a39787971bcc3d6ba73996
• Attack contract: 0x5992f10a5b284be845947a1ae1694f8560a89fa8

Attack Details

Attacker borrowed 1000 ETH through flash loan from Balancer, and then borrowed 20 Doodles NFT from NFTX flash loan, using the borrowed ETH.

As shown on the image above, the attacker:

1. Called supplyERC721function in the Pool contract 0xebe72cdafebc1abf26517dd64b28762df77912a9to deposit three Doodles (720, 5251, 7425) and received nToken (nDoodles) as the proof of deposit.
2. According to the pricing Oracle, Doodles price is 13.5 ETH, and attacker borrowed 12.15 WETH.
3. Called withdrawERC721function in the Pool contract 0xebe72cdafebc1abf26517dd64b28762df77912a9to take back two of the three Doodles (720 and 5251).

Note: **safeTransferFrom**below has **Reentrancy** risk and will be mentioned later.

During the collateral transfer, attack contract executed malicious onERC721Received function and called liquidationERC721function from Pool contract 0xebe72cdafebc1abf26517dd64b28762df77912a9to liquidate:

Reentrancy Attack

The risk is within the liquidation logic:

Within liquidation function executeERC721LiquidationCallin the LiquidationLogiccontract, attacker paid back 12.15 WETH borrowed, and executed code logistics above:

1. Called _burnCollateralNTokensto:

• Burn proof of deposit nToken (nDoodles)
• CallsafeTransferFromto return collateral Doodles

2. Called setBorrowingto set the borrowing state to false

This is where the issue occurs: the code does not follow the **Checks-Effects-Interactions** code pattern and brings in **Reentrancy** risk.

How it Happens

Attack contract implemented onERC721Receivedfunction to accomplish reentrancy, which executed logics below just before setBorrowing(..., false) above:

• Attacker deposited ALL 20 Doodles and borrowed 81 WETH. As shown below, the borrowing function also called setBorrowingfunction to set the borrowing state as true

After the execution of staking and borrowing, it went back to the previous liquidating logic to continue executing setBorrowing(..., false)

Apparently, the continuous calling of **setBorrowing** overwrites the borrowing record to **false**, and thus erased the borrow record and cleared the borrow collateral.

Then the attacker called withdrawERC721 function to try taking out Doodles and, as the Borrowing state was already false(considered as no debts), it bypassed the check and successfully took away all Doodles.

Recommended Fix

Why does **_Reentrancy_** Happened?

According to EIP-721, when utilizing safeTransferFrom function to transfer NFT, if the target address is a contract, it will call onERC721Received function in this contract and require a specific value to be returned from the function.

The reason for this requirement is to make sure the contract is compatible with NFT transfer operations and prevent the NFT from getting transferred to incompatible non-transferrable contracts.

However, it also introduces security risks such that attackers can put malicious code into onERC721Received function.

If a given contract does not consider this risk, and neither apply [**ReentrancyGuard**](https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard) nor following the **check-effects-interactions** pattern, it becomes vulnerable to reentrancy attack.

How to Fix

In this case, we suggest to use [**ReentrancyGuard**](https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard), and follow the **checks-effects-interactions** pattern during implementation:

• Set borrowing state **setBorrowing** before transfer NFT in**executeERC721LiquidationCall**function.

About Secure3

Secure 3 aims to secure Web3 by empowering a transparent, collaborative and verifiable security ecosystem. We aim to provide trustworthy security auditing for projects, competitive incentive model for auditors, and verifiable auditing track record for Web3 community.