LooksRare Aggregator contest - perseverancesuccess's results

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L46 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L153-L163 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L228

Vulnerability details

Impact

The smart contracts that integrate with LooksRareAggregator fail all the transactions if the proxy fee is updated to be higher value. One more impact that is it is difficult for the other contracts or normal users to find the fee basis points to have the successful trading.

Currently the document states that "Before executing a trade, the client should fetch each proxy's fee data and include the returned basis points in the trade data's maxFeeBp parameter to prevent the fee basis points from suddenly increasing beyond the buyer's acceptable fee levels". But in the implementation of the contract: the _proxyFeeData is private data and there is no get function to get the fee.

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

mapping(address => FeeData) private _proxyFeeData;

There is the setFee function so the owner can update the fees to a higher values.

function setFee(
        address proxy,
        uint256 bp,
        address recipient
    ) external onlyOwner {
        if (bp > 10000) revert FeeTooHigh();
        _proxyFeeData[proxy].bp = bp;
        _proxyFeeData[proxy].recipient = recipient;

        emit FeeUpdated(proxy, bp, recipient);
    }

If the other users constructed the transaction based on the old feeData, then the check at Line228 will return maxFeeBpViolated is true, then the transaction will skip the trade if the isAtomic input is false, or revert if isAtomic input is true.

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

maxFeeBpViolated = singleTradeData.maxFeeBp < feeData.bp;

This is a medium issue because the core functionality of the protocol is impacted. Since there is no way to get the _proxyFeeData, then it is very difficult for other contracts to integrate LooksRareAggregator to create a successful trade.

The below scenario is possible. The other contract implemented the accept the fee level of "x" amount of fees based on the current documentation. But The owner of LooksRareAggregator update fees to a higher value than x => Then all the trade transactions will fail. The other contract should update the implementation.

Proof of concept:

The following scenarios is possible

Step 0: The client contract implement the accept the fee level of "x" amount based on the current documentation Step 1: The The owner of LooksRareAggregator update fees to a higher value than x Step 2: The client contract fails all transactions. Step 3: The client contract should wait for the new announcement of the fees from the LooksRare team or investigate the failures by transactions history of LooksRareAggregator contract.

The POC written in Foundry: testExecuteHigherFee in file LooksRareAggregatorTrades.t.sol: Just replace the current file in test folder with this new file https://drive.google.com/file/d/1NNawMrXzRRntmbdGuDrybMQQ6x9QdHAm/view?usp=sharing

To run test

FOUNDRY_PROFILE=local forge test -m testExecuteHigherFee --use solc:0.8.17

The Log of the test failed: https://drive.google.com/file/d/1_HMN6wXIIPc8kYjTdBhE417_KT0AAEMD/view?usp=sharing The log of the test case with correct feeData and passed: https://drive.google.com/file/d/1SMBvHw1DodLNtrld0VzLzXK8Yhwcm1Ql/view?usp=share_link

So notice that the test case testExecuteReentrancy passed. In the test case testExecuteHigherFee the _proxyFeeData.bp updated by the owner has higher fee , the transaction with same trade data as in testExecuteReentrancy but the transaction fails.

The git diff vs commit e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f is here: https://drive.google.com/file/d/1moLcr6-I59I6OAXbjLMFGwrMqCQ-ynP_/view?usp=sharing To apply the patch

git apply POC.patch

Tools used

VSCode, Foundry

Recommended Mitigation Steps

Should change the _proxyFeeData to public or implement the getter functionality