LooksRare Aggregator contest - jayphbee's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L35 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L46 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L57

Vulnerability details

Impact

The call opcode's return value not checked, which could leads to the originator lose funds.

Proof of Concept

The caller of LooksRareAggregator.sol::execute could be a contract who may not implement the fallback or receive function, when a call to it with value sent, it will revert, thus failed to receive the ETH.

Let's imagine the contract call the execute function to buy multiple NFTs with ETH as the payout currency and make the isAtomic parameter being false. Since the batch buy of NFTs is not atomic, the failed transactions in LooksRare or Seaport marketplace will return the passed ETH. The contract doesn't implement the fallback/receive function and the call opcode's return value not checked, thus the ETH value will be trapped in the LooksRareAggregator contract until the next user call the execute function and the trapped ETH is returned to him. The originator lose funds.

    function _returnETHIfAny(address recipient) internal {
        assembly {
            if gt(selfbalance(), 0) {
                let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0) // @audit-issue status not checked.
            }
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

check the return value the call opcode.

    function _returnETHIfAny() internal {
        bool status;
        assembly {
            if gt(selfbalance(), 0) {
                status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0) // @audit-issue [MED] status not checked
            }
        }
        if (!status) revert ETHTransferFail();
    }

    function _returnETHIfAny(address recipient) internal {
        bool status;
        assembly {
            if gt(selfbalance(), 0) {
                status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0) // @audit-issue status not checked.
            }
        }
        if (!status) revert ETHTransferFail();
    }
function _returnETHIfAnyWithOneWeiLeft() internal {
        bool status;
        assembly {
            if gt(selfbalance(), 1) {
                status := call(gas(), caller(), sub(selfbalance(), 1), 0, 0, 0, 0)
            }
        }
        if (!status) revert ETHTransferFail();
    }

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenTransferrer.sol#L22

Vulnerability details

Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Proof of Concept

In the TokenTransferer.sol::TokenTransferer::_transferTokenToReceipient function the transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. There are reasons not use transferFrom function:

• OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible.
• There are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Tools Used

Manual review

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for ERC721 transfers.

        if (collectionType == CollectionType.ERC721) {
            IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);
        } else if (collectionType == CollectionType.ERC1155) {
            IERC1155(collection).safeTransferFrom(address(this), recipient, tokenId, amount, "");
        }