LooksRare Aggregator contest - hanxin's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L244-L245

Vulnerability details

Impact

When using any ERC20 token to purchase NFT, after the purchase is successful, the 108th line of code in the contract LooksRareAggregator determines whether there are any remaining unused ERC20 tokens. If there is any remaining, it will be returned to the address specified by the originator. The code is as follows: if (tokenTransfersLength > 0) _returnERC20TokensIfAny(tokenTransfers, originator);

There is nothing wrong with the logic at this moment, the real problem is that in the _returnERC20TokensIfAny function, a fatal error was made when performing the refund operation:

1. There is no check whether the value of balance is greater than the amount of ERC20 tokens transferred by the originator
2. Recklessly transfer all the balance of the ECR20 token back to the originator

Hackers will use the above loopholes to transfer the entire balance of an ERC20 token in the contract

Proof of Concept

Error code location: https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L244-L245

Attack steps:

1. Suppose there is a token named A and the address is 0xAAAA 2.The attacker deliberately exceeded the price and transferred token A to the contract ERC20EnabledLooksRareAggregator to purchase a certain NFT
2. The LooksRareAggregator contract helps the attacker to buy the desired NFT on opensea(just for example)
3. When the LooksRareAggregator finds that the attacker's token A has surplus, it will transfer all the tokens A in the contract (regardless of the amount and whether it belongs to the attacker's own token) to the originator address designated by the attacker
4. If there are other users' token A balances in the contract at this time, they will also be transferred to the originator address designated by the attacker!

Tools Used

Recommended Mitigation Steps

In the _returnERC20TokensIfAny function, the amount of ERC20 tokens to be returned should be strictly judged, and the current balance of each user's ERC20 tokens needs to be stored for logical judgment (you can use a mapping structure or build a Merkle tree to store )