Platform: Code4rena
Start Date: 08/11/2022
Pot Size: $60,500 USDC
Total HM: 6
Participants: 72
Period: 5 days
Judge: Picodes
Total Solo HM: 2
Id: 178
League: ETH
Rank: 20/72
Findings: 3
Award: $264.89
🌟 Selected for report: 0
🚀 Solo Findings: 0
151.3257 USDC - $151.33
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49
When the buyer uses a contract for purchasing NFTs, such contract can call the LooksRareAggregator.execute
function to purchase an NFT using ETH. If the sent ETH amount when calling this function is not entirely used after the NFT trade is executed, then the _returnETHIfAny(address recipient)
function call will attempt to return the unused ETH amount back to the originator
, which should be the buyer contract, through executing let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0)
. As this low-level call executes the logics in the buyer contract's receive
function, if calling this receive
function reverts, such as when this receive
function further calls an external contract's function that is temporarily paused which is beyond the buyer's control, then this low-level call will fail silently because the _returnETHIfAny(address recipient)
function does not have any code that requires the status
returned by this low-level call to be true
. In this case, since calling the _returnETHIfAny(address recipient)
function does not revert, the unused ETH amount fails to be returned to the buyer contract and remains trapped in the LooksRareAggregator
contract. As mentioned by my other finding titled "ETH amount that is trapped in LooksRareAggregator
contract can be withdrawn by user who is not LooksRareAggregator
's owner", it is possible that another user, who calls the LooksRareAggregator.execute
function before the LooksRareAggregator
contract's owner calls the rescueETH
function, gains such trapped ETH amount; hence, the buyer contract can lose the unused ETH amount sent by it after such amount becomes trapped in the contract.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112
function execute( TokenTransfer[] calldata tokenTransfers, TradeData[] calldata tradeData, address originator, address recipient, bool isAtomic ) external payable nonReentrant { ... _returnETHIfAny(originator); emit Sweep(originator); }
function _returnETHIfAny(address recipient) internal { assembly { if gt(selfbalance(), 0) { let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0) } } }
First, please add test\foundry\utils\MockBuyer.sol
as follows.
// SPDX-License-Identifier: MIT pragma solidity 0.8.17; contract MockBuyer { receive() external payable { // This MockBuyer contract simulates a situation where executing logics in the buyer contract's receive function reverts; // for example, such logics could include an external function call that reverts // because the corresponding function in the external contract is temporarily paused. revert(); } }
Then, please add the following code in test\foundry\LooksRareAggregatorTrades.t.sol
.
MockBuyer.sol
as follows.import "./utils/MockBuyer.sol";
function testETHAmountSentByBuyerContractThatIsUnusedAfterPurchasingNFTCanBeTrappedInAggregator() public { vm.createSelectFork(vm.rpcUrl("mainnet"), 15_282_897); aggregator = new LooksRareAggregator(); vm.deal(address(aggregator), 0); LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator)); vm.deal(address(looksRareProxy), 0); aggregator.addFunction(address(looksRareProxy), LooksRareProxy.execute.selector); TokenTransfer[] memory tokenTransfers = new TokenTransfer[](0); BasicOrder[] memory validOrders = validBAYCOrders(); BasicOrder[] memory orders = new BasicOrder[](1); orders[0] = validOrders[0]; bytes[] memory ordersExtraData = new bytes[](1); ordersExtraData[0] = abi.encode(orders[0].price, 9550, 0, LOOKSRARE_STRATEGY_FIXED_PRICE); ILooksRareAggregator.TradeData[] memory tradeData = new ILooksRareAggregator.TradeData[](1); tradeData[0] = ILooksRareAggregator.TradeData({ proxy: address(looksRareProxy), selector: LooksRareProxy.execute.selector, value: orders[0].price, maxFeeBp: 0, orders: orders, ordersExtraData: ordersExtraData, extraData: "" }); // The buyer uses the mockBuyer contract for purchasing NFTs. // It simulates a situation where executing logics in the buyer contract's receive function reverts; // for example, such logics could include an external function call that reverts // because the corresponding function in the external contract is temporarily paused. MockBuyer mockBuyer = new MockBuyer(); vm.deal(address(mockBuyer), orders[0].price + 1 ether); // the aggregator owns 0 ETH at this moment assertEq(address(aggregator).balance, 0); // The mockBuyer contract calls the LooksRareAggregator.execute function to purchase the NFT using ETH // while sending an ETH amount that is more than the NFT's purchase price. vm.prank(address(mockBuyer)); aggregator.execute{value: address(mockBuyer).balance}(tokenTransfers, tradeData, address(mockBuyer), address(mockBuyer), false); // Although the mockBuyer contract receives the purchased NFT after calling the LooksRareAggregator.execute function, // the difference between the sent ETH amount and the NFT's purchase price is trapped in the aggregator // instead of being returned to the mockBuyer contract. assertEq(IERC721(BAYC).ownerOf(7139), address(mockBuyer)); assertEq(address(mockBuyer).balance, 0); assertEq(address(aggregator).balance, 1 ether); }
VSCode
Similar to the _transferETH
function, the _returnETHIfAny(address recipient)
function (https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelETH.sol#L43-L49) can be updated to the following code.
function _returnETHIfAny(address recipient) internal { bool status = true; assembly { if gt(selfbalance(), 0) { status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0) } } if (!status) revert ETHTransferFail(); }
Although the _returnETHIfAny()
and _returnETHIfAnyWithOneWeiLeft
functions are not called by the contracts in scope, these functions can be updated to be similar to the _transferETH
function as well. The current implementations of these functions are shown below.
function _returnETHIfAny() internal { assembly { if gt(selfbalance(), 0) { let status := call(gas(), caller(), selfbalance(), 0, 0, 0, 0) } } }
function _returnETHIfAnyWithOneWeiLeft() internal { assembly { if gt(selfbalance(), 1) { let status := call(gas(), caller(), sub(selfbalance(), 1), 0, 0, 0, 0) } } }
function _transferETH(address _to, uint256 _amount) internal { bool status; assembly { status := call(gas(), _to, _amount, 0, 0, 0, 0) } if (!status) revert ETHTransferFail(); }
#0 - c4-judge
2022-11-19T11:27:16Z
Picodes marked the issue as duplicate of #241
#1 - c4-judge
2022-12-16T13:58:32Z
Picodes changed the severity to 2 (Med Risk)
#2 - c4-judge
2022-12-16T13:58:33Z
Picodes marked the issue as satisfactory
77.2215 USDC - $77.22
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L34-L38 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/ERC20EnabledLooksRareAggregator.sol#L28-L37 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L241-L251 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L46-L57
It is possible that an ERC20 token amount is trapped in the LooksRareAggregator
contract; for instance, a user can accidentally send some of an ERC20 token to this contract. Because the LooksRareAggregator
contract inherits the TokenRescuer
contract, the LooksRareAggregator
contract's owner is able to call the rescueERC20
function to transfer such trapped ERC20 token amount to an appropriate address. However, after noticing that there is an ERC20 token amount being trapped in the LooksRareAggregator
contract and the contract's owner has not yet called the rescueERC20
function for the corresponding token, a user can call the ERC20EnabledLooksRareAggregator.execute
function to, for example, purchase an NFT using ETH while setting the tokenTransfers
input to include 0 of the trapped ERC20 token. In this way, when the ERC20EnabledLooksRareAggregator.execute
function calls the LooksRareAggregator.execute
function, which executes the _returnERC20TokensIfAny
and _executeERC20DirectTransfer
functions for the trapped ERC20 token, this user is able to withdraw the LooksRareAggregator
contract's balance of the trapped ERC20 token that includes such token's trapped amount. After this user calls the ERC20EnabledLooksRareAggregator.execute
function, the LooksRareAggregator
contract's owner's rescueERC20
function call would revert since the LooksRareAggregator
contract's balance of the trapped ERC20 token has become 0 and executing IERC20(currency).balanceOf(address(this)) - 1
underflows. In this situation, the LooksRareAggregator
contract's owner is not able to transfer the trapped ERC20 token amount, which violates the design. Moreover, the user, who calls the ERC20EnabledLooksRareAggregator.execute
function in this case, gains the trapped ERC20 token amount, which is unfair to the user, who sends the trapped ERC20 token amount and loses all of it.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L34-L38
function rescueERC20(address currency, address to) external onlyOwner { uint256 withdrawAmount = IERC20(currency).balanceOf(address(this)) - 1; if (withdrawAmount == 0) revert InsufficientAmount(); _executeERC20DirectTransfer(currency, to, withdrawAmount); }
function execute( TokenTransfer[] calldata tokenTransfers, ILooksRareAggregator.TradeData[] calldata tradeData, address recipient, bool isAtomic ) external payable { if (tokenTransfers.length == 0) revert UseLooksRareAggregatorDirectly(); _pullERC20Tokens(tokenTransfers, msg.sender); aggregator.execute{value: msg.value}(tokenTransfers, tradeData, msg.sender, recipient, isAtomic); }
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L51-L112
function execute( TokenTransfer[] calldata tokenTransfers, TradeData[] calldata tradeData, address originator, address recipient, bool isAtomic ) external payable nonReentrant { ... if (tokenTransfersLength > 0) _returnERC20TokensIfAny(tokenTransfers, originator); ... }
function _returnERC20TokensIfAny(TokenTransfer[] calldata tokenTransfers, address recipient) private { uint256 tokenTransfersLength = tokenTransfers.length; for (uint256 i; i < tokenTransfersLength; ) { uint256 balance = IERC20(tokenTransfers[i].currency).balanceOf(address(this)); if (balance > 0) _executeERC20DirectTransfer(tokenTransfers[i].currency, recipient, balance); unchecked { ++i; } } }
function _executeERC20DirectTransfer( address currency, address to, uint256 amount ) internal { (bool status, bytes memory data) = currency.call(abi.encodeWithSelector(IERC20.transfer.selector, to, amount)); if (!status) revert ERC20TransferFail(); if (data.length > 0) { if (!abi.decode(data, (bool))) revert ERC20TransferFail(); } }
Please add the following code in test\foundry\LooksRareAggregatorTrades.t.sol
.
stdError
and ERC20EnabledLooksRareAggregator
as follows.import {stdError} from "../../lib/forge-std/src/StdError.sol"; import {ERC20EnabledLooksRareAggregator} from "../../contracts/ERC20EnabledLooksRareAggregator.sol";
function testERC20TrappedInAggregatorCanBeWithdrawnByNonOwner() public { vm.createSelectFork(vm.rpcUrl("mainnet"), 15_282_897); aggregator = new LooksRareAggregator(); vm.deal(address(aggregator), 0); LooksRareProxy looksRareProxy = new LooksRareProxy(LOOKSRARE_V1, address(aggregator)); vm.deal(address(looksRareProxy), 0); aggregator.addFunction(address(looksRareProxy), LooksRareProxy.execute.selector); ERC20EnabledLooksRareAggregator erc20EnabledAggregator = new ERC20EnabledLooksRareAggregator(address(aggregator)); aggregator.setERC20EnabledLooksRareAggregator(address(erc20EnabledAggregator)); BasicOrder[] memory validOrders = validBAYCOrders(); BasicOrder[] memory orders = new BasicOrder[](1); orders[0] = validOrders[0]; bytes[] memory ordersExtraData = new bytes[](1); ordersExtraData[0] = abi.encode(orders[0].price, 9550, 0, LOOKSRARE_STRATEGY_FIXED_PRICE); ILooksRareAggregator.TradeData[] memory tradeData = new ILooksRareAggregator.TradeData[](1); tradeData[0] = ILooksRareAggregator.TradeData({ proxy: address(looksRareProxy), selector: LooksRareProxy.execute.selector, value: orders[0].price, maxFeeBp: 0, orders: orders, ordersExtraData: ordersExtraData, extraData: "" }); MockERC20 mockERC20 = new MockERC20(); // alice owns 1 ether mockERC20 at this moment address alice = address(1); mockERC20.mint(alice, 1 ether); // alice accidentally sends 1 ether mockERC20 to the aggregator vm.prank(alice); mockERC20.transfer(address(aggregator), 1 ether); vm.deal(_buyer, orders[0].price); // After noticing that the aggregator at this moment owns the 1 ether mockERC20 that was accidentally sent by alice // and the LooksRareAggregator.rescueERC20 function is not yet called by the aggregator's owner, // _buyer calls the ERC20EnabledLooksRareAggregator.execute function to purchase the NFT using ETH // while setting the tokenTransfers input to include 0 mockERC20. vm.startPrank(_buyer); TokenTransfer[] memory tokenTransfers = new TokenTransfer[](1); tokenTransfers[0].amount = 0; tokenTransfers[0].currency = address(mockERC20); erc20EnabledAggregator.execute{value: orders[0].price}(tokenTransfers, tradeData, _buyer, false); vm.stopPrank(); // After the ERC20EnabledLooksRareAggregator.execute function is called, // _buyer receives the purchased NFT and also the 1 ether mockERC20 that was accidentally sent by alice. assertEq(IERC721(BAYC).ownerOf(7139), _buyer); assertEq(mockERC20.balanceOf(_buyer), 1 ether); // Calling the LooksRareAggregator.rescueERC20 function by the aggregator's owner reverts // because subtracting 1 from the aggregator's mockERC20 balance, which is 0 at this moment, underflows. vm.expectRevert(stdError.arithmeticError); aggregator.rescueERC20(address(mockERC20), alice); // the aggregator's owner has failed to recover the 1 ether mockERC20, which was accidentally sent, for alice assertEq(mockERC20.balanceOf(alice), 0); }
VSCode
At the beginning of the LooksRareAggregator.execute
function, for each ERC20 token in the tokenTransfers
input, IERC20(tokenTransfers[i].currency).balanceOf(address(this)) - tokenTransfers[i].amount
can be executed to get the LooksRareAggregator
contract's balance of the tokenTransfers[i].currency
token before receiving the corresponding token amount from the originator
; then, for each of these ERC20 tokens, such balance amount should be excluded when calling the _returnERC20TokensIfAny
and _executeERC20DirectTransfer
functions for transferring the token amount that is left back to the originator
.
#0 - c4-judge
2022-11-19T11:32:27Z
Picodes marked the issue as duplicate of #277
#1 - c4-judge
2022-12-16T13:58:21Z
Picodes changed the severity to 2 (Med Risk)
#2 - c4-judge
2022-12-16T13:58:21Z
Picodes marked the issue as satisfactory
🌟 Selected for report: RaymondFam
Also found by: 0x1f8b, 0x52, 0xSmartContract, 0xc0ffEE, 0xhacksmithh, 8olidity, Awesome, BClabs, Bnke0x0, Chom, Deivitto, Hashlock, IllIllI, Josiah, KingNFT, Nyx, R2, ReyAdmirado, Rolezn, SamGMK, Sathish9098, SinceJuly, V_B, Vadis, Waze, a12jmx, adriro, ajtra, aphak5010, bearonbike, bin, brgltd, carlitox477, carrotsmuggler, cccz, ch0bu, chaduke, datapunk, delfin454000, erictee, fatherOfBlocks, fs0c, horsefacts, jayphbee, ktg, ladboy233, pashov, perseverancesuccess, rbserver, ret2basic, tnevler, zaskoh
36.3434 USDC - $36.34
rescueERC721
FUNCTION CAN POSSIBLY LOCK THE TRANSFERRED ERC721 TOKENCalling the rescueERC721
function further calls the _executeERC721TransferFrom
function. Calling the _executeERC721TransferFrom
function then executes (bool status, ) = collection.call(abi.encodeWithSelector(IERC721.transferFrom.selector, from, to, tokenId))
. Since the IERC721.transferFrom
function selector is used, if the address corresponding to the to
input is a contract that does not support ERC721 protocol, then it is possible that the ERC721 token is transferred to an address that cannot interact with the corresponding ERC721 contract. As a result, the transferred ERC721 is locked and cannot be retrieved.
For reference, OpenZeppelin's documentation for transferFrom
states: "
Note that the caller is responsible to confirm that the recipient is capable of receiving ERC721 or else they may be permanently lost." Please consider updating the _executeERC721TransferFrom
function to use the corresponding IERC721.safeTransferFrom
function selector instead of the IERC721.transferFrom
function selector.
function rescueERC721( address collection, address to, uint256 tokenId ) external onlyOwner { _executeERC721TransferFrom(collection, address(this), to, tokenId); }
function _executeERC721TransferFrom( address collection, address from, address to, uint256 tokenId ) internal { (bool status, ) = collection.call(abi.encodeWithSelector(IERC721.transferFrom.selector, from, to, tokenId)); if (!status) revert ERC721TransferFromFail(); }
rescueERC721
, rescueERC1155
, rescueETH
, and rescueERC20
FUNCTIONSThe LooksRareAggregator
contract's owner has the privilege to call the rescueERC721
, rescueERC1155
, rescueETH
, and rescueERC20
functions to transfer the corresponding trapped assets to the specified addresses. Looking at https://docs.looksrare.org/developers/deployed-contract-addresses, it appears to be that this protocol does not have a governance contract that lets the community to vote. This means that the LooksRareAggregator
contract's owner cannot be such governance contract currently, and there is no guarantee that the LooksRareAggregator
contract's current owner will not become compromised or malicious in the future. When the LooksRareAggregator
contract's owner becomes compromised or malicious, this owner can call the rescueERC721
, rescueERC1155
, rescueETH
, and rescueERC20
contracts to immediately withdraw the corresponding trapped assets so these assets would be lost forever. Please consider setting up a governance contract and transferring the ownership of the LooksRareAggregator
contract to this governance contract so the community can vote on how these rescueERC721
, rescueERC1155
, rescueETH
, and rescueERC20
functions can be called.
function rescueERC721( address collection, address to, uint256 tokenId ) external onlyOwner { _executeERC721TransferFrom(collection, address(this), to, tokenId); }
function rescueERC1155( address collection, address to, uint256[] calldata tokenIds, uint256[] calldata amounts ) external onlyOwner { _executeERC1155SafeBatchTransferFrom(collection, address(this), to, tokenIds, amounts); }
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L22-L26
function rescueETH(address to) external onlyOwner { uint256 withdrawAmount = address(this).balance - 1; if (withdrawAmount == 0) revert InsufficientAmount(); _transferETH(to, withdrawAmount); }
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenRescuer.sol#L34-L38
function rescueERC20(address currency, address to) external onlyOwner { uint256 withdrawAmount = IERC20(currency).balanceOf(address(this)) - 1; if (withdrawAmount == 0) revert InsufficientAmount(); _executeERC20DirectTransfer(currency, to, withdrawAmount); }
address(0)
CHECKS FOR CRITICAL ADDRESS INPUTSTo prevent unintended behaviors, critical address inputs should be checked against address(0)
.
Please consider checking _aggregator
in the following constructor.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/ERC20EnabledLooksRareAggregator.sol#L21-L23
constructor(address _aggregator) { aggregator = ILooksRareAggregator(_aggregator); }
Please consider checking _marketplace
and _aggregator
in the following constructor.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/LooksRareProxy.sol#L37-L40
constructor(address _marketplace, address _aggregator) { marketplace = ILooksRareExchange(_marketplace); aggregator = _aggregator; }
Please consider checking _marketplace
and _aggregator
in the following constructor.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/SeaportProxy.sol#L45-L48
constructor(address _marketplace, address _aggregator) { marketplace = SeaportInterface(_marketplace); aggregator = _aggregator; }
TokenRescuer
is imported twice in LooksRareAggregator.sol
. Please remove one of these import
code lines.
https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L11-L17
import {TokenRescuer} from "./TokenRescuer.sol"; ... import {TokenRescuer} from "./TokenRescuer.sol";
To improve readability and maintainability, constants can be used instead of magic numbers. Please note that the following magic numbers are not flagged in https://gist.github.com/Picodes/0554505ff357a2da3bca5fbb839ee7da.
Please consider replacing the following magic numbers, such as 2
, with constants.
contracts\LooksRareAggregator.sol 158: if (bp > 10000) revert FeeTooHigh(); contracts\proxies\SeaportProxy.sol 147: uint256 orderFee = (orders[i].price * feeBp) / 10000; 207: uint256 orderFee = (price * feeBp) / 10000; 246: offer[0].itemType = ItemType(uint8(order.collectionType) + 2);
NatSpec comments provide rich code documentation. The @param comment is missing for the following _returnETHIfAny
function. Please consider completing the NatSpec comment for it.
function _returnETHIfAny(address recipient) internal { assembly { if gt(selfbalance(), 0) { let status := call(gas(), recipient, selfbalance(), 0, 0, 0, 0) } } }
NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.
contracts\ERC20EnabledLooksRareAggregator.sol 39: function _pullERC20Tokens(TokenTransfer[] calldata tokenTransfers, address source) private { contracts\LooksRareAggregator.sol 222: function _encodeCalldataAndValidateFeeBp( 241: function _returnERC20TokensIfAny(TokenTransfer[] calldata tokenTransfers, address recipient) private { contracts\TokenTransferrer.sol 14: function _transferTokenToRecipient( contracts\proxies\LooksRareProxy.sol 107: function _executeSingleOrder( contracts\proxies\SeaportProxy.sol 88: function _executeAtomicOrders( 136: function _handleFees( 166: function _transferFee( 178: function _executeNonAtomicOrders( 227: function _populateParameters(BasicOrder calldata order, OrderExtraData memory orderExtraData)
#0 - c4-judge
2022-11-21T17:13:23Z
Picodes marked the issue as grade-b
#1 - 0xhiroshi
2022-11-24T18:26:47Z
only N-01 is valid
#2 - c4-sponsor
2022-11-24T18:26:51Z
0xhiroshi requested judge review
#3 - c4-judge
2022-12-11T13:31:22Z
Picodes marked the issue as grade-a
#4 - c4-judge
2022-12-11T22:23:00Z
Picodes marked the issue as grade-b