DittoETH

Summary

Remember that the OpenZeppelin implementation of ERC721 and ERC1155 vulnerable to reentrancy attacks, since safeTransferFrom functions perform an external call to the user address (onReceived)! we have to do to exploit this is deploy a contract where onERC721Received calls back into the safeTransferFrom function.

Vulnerability Details

Reentrancy is an attack that can occur when a bug in a contract may allow a malicious contract to reenter the contract unexpectedly during execution of the original function. This can be used to drain funds from a smart contract if used maliciously. Reentrancy is likely the single most impactful vulnerability in terms of total loss of funds by smart contract hacks, and should be considered accordingly. List of reentrancy attacks

Reentrancy can only happen when your smart contract calls another smart contract via function call or sending ether.

If you do not call another contract or send ether in the middle of an execution, you cannot hand over execution control, and reentrancy cannot happen.

The tricky part is that you might not always know when you are calling another contract. For example, this code is actually re-entrant if this is used inside of an ERC1155 or ERC721 contract.

The most common NFT standards are ERC-721 and ERC-1155. OpenZeppelin has implemented corresponding template contracts that are commonly used by the community. These contracts incorporate functions such as _safeTransfer, _safeMint, _mint, _safeTransferFrom, _mintBatch and _safeBatchTransferFrom that will invoke the receiver contract through the callback functions onERC721Received, onERC1155Received, and onERC1155BatchReceived to ensure that the tokens can be received by the receiver. However, this can potentially create a security loophole. An attacker can perform a reentrancy call inside the onERC721Received, onERC1155Received, or onERC1155BatchReceived callback.

If malicious code is included in onERC721Received() or onERC1155Received in the target contract of the transfer or mint, a reentrancy attack may be conducted.

Using a safe function does not guarantee a safe contract

Confusingly enough, the word “safe” means it is checking if the receiving address is a smart contract, then attempting to call the “onERC721Received” function. The transferFrom and _mint functions don’t do this, so you don’t have to worry about re-entrancy.

This doesn’t mean you shouldn’t use safeTransferFrom or _safeMint methods, it means you should use the check-effects pattern or reentrancy guards to prevent re-entrancy if you use it.

https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ERC721Facet.sol?plain=1#L93

I noticed that the safeTransferFrom() function used in line 94, as seen below, has a special callback mechanism embedded, which could be exploited to hijack the control flow.

Specifically, in the OpenZeppelin’s ERC721 implementation, while safeTransferFrom function , the onERC721Received() external function is invoked when the receiving address is a contract . This is the key to re-enter the vulnerable contract.

However, this external function call creates a security loophole. Specifically, the attacker can perform a reentrant call inside the onERC721Received callback.
All we have to do to exploit this is deploy a contract where onERC721Received calls back into the safeTransferFrom function. Because the original safeTransferFrom call hasn’t completed

Impact

A bug that could cause significant User funds to be lost or stolen directly

Tools Used

Manual code review

Recommendations

A common practice is to use the Mutex pattern, also known as Reentrancy Guard. This method leverages the usage of modifiers (in combination with some checks) to lock the contract and ensure that a given function cannot be reentered until finishes its execution and the contract is unlocked.

Use Openzeppelin or Solmate Re-Entrancy pattern.

or use