code-423n4 / 2023-06-lukso-findings

3 stars 1 forks source link

ERC721 onERC721Received() reentrancy #21

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.sol?plain=1#L246-L257 https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.sol?plain=1#L307-L339 https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721InitAbstract.sol?plain=1#L246-L257 https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721InitAbstract.sol?plain=1#L307-L339

Vulnerability details

Impact

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

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, 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.

Resources

https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/reentrancy.md#safemint

https://samczsun.com/the-dangers-of-surprising-code/

https://twitter.com/Beosin_com/status/1565180106682671104?lang=en

Proof of Concept

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

function _safeTransfer(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _transfer(from, to, bytes32(tokenId), true, data);
        require(
            _checkOnERC721Received(from, to, tokenId, data),
            "LSP8CompatibleERC721: transfer to non ERC721Receiver implementer"
        );
    }

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

function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.code.length > 0) {
            try
                IERC721Receiver(to).onERC721Received(
                    msg.sender,
                    from,
                    tokenId,
                    data
                )
            returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert(
                        "LSP8CompatibleERC721: transfer to non ERC721Receiver implementer"
                    );
                } else {
                    // solhint-disable no-inline-assembly
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

However, this external function call creates a security loophole. Specifically, the attacker can perform a reentrant call inside the onERC721Received callback. For instance, in the vulnerable LSP8CompatibleERC721 contract, the attacker can invoke the safeTransferFrom function again in the onERC721Received callback

For safety, this function performs a callback to the recipient of the token to check that they're willing to accept the transfer. However, we're the recipient of the token, which means we just got a callback at which point we can do whatever we like, including calling safeTransferFrom again. If we do this, we'll reenter the function

Without reentrancy guard, onERC721Received will allow an attacker controlled contract to call the transfer again, which may not be desirable to some parties, like allowing transfering more than allowed.

Tools Used

Recommended Mitigation Steps

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.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.12;

contract ReEntrancyGuard {
    bool internal locked;

    modifier noReentrant() {
        require(!locked, "No re-entrancy");
        locked = true;
        _;
        locked = false;
    }
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) noReentrant public virtual {
        _safeTransfer(from, to, tokenId, data);
    }
}

Assessed type

Reentrancy

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

Insufficient proof

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Insufficient proof