Royalty stealing

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L99 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L152 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L254

Vulnerability details

Impact

If an author of an NFT does a trade in a private pool where royalty payments are enabled, then the trade is effectively happening at a discount for the author, as they receive a commission from each trade. Manipulation of the pool configuration allows for unexpected slippage for the authors of the NFTs.

EthRouter's buy(), sell(), and change() functions allow for the payRoyalties flag to be set, but do not enforce it if the transaction goes through PrivatePool. This allows the pool owner to frontrun transactions of people who think that part of their funds will go towards paying royalties, when in reality the pool owner sandwiches their transaction and takes the royalties as positive slippage.

Why the vulnerability is rated as high severity. Suppose the trade is done by the author of an NFT who wants to buy their NFT from a private pool, and the router asks for 95 ETH to be paid for the NFT + 5 ETH for royalty. Let's assume that the protocol and pool fees are zero, and the royalty is 5 ETH. The NFT author expects that although the router asks for 100 ETH, in reality, due to royalty payments to themselves, they will acquire it for 95 ETH.

A malicious pool owner sandwiches the NFT author's transaction (see the PoC) and takes the royalties for themselves. The NFT author purchases their own NFT for 100 ETH, even though they expected the transaction to be completed at a discount of 5 ETH.

We see that a user can lose significant funds in a situation where they clearly do not expect it. This is precisely why the vulnerability has been assigned a HIGH status.

Proof of Concept

As an example, here is a more detailed attack scenario for the case when the victim calls EthRouter.buy(..., payRoyalties=true) and the trade goes through a malicious owner's private pool. The owner sandwiches the victim's transaction:

Part 1. The owner sets setPayRoyalties(false) and moves the price of the NFT in the pool up. They can do this either through an actual trade or by setting incorrect values in setVirtualReserves(). They can also increase the fee through setFeeRate().

Part 2. The victim's transaction is then executed, in which they purchase at a higher price but without paying royalties.

The router has the following code:

// refund any surplus ETH to the caller
if (address(this).balance > 0) {
    msg.sender.safeTransferETH(address(this).balance);
}

In this case, it does not work because all the sent funds were eaten up by the higher price in the pool.

Part 3. The owner returns the pool to normal and fixes the profit.

To do this, they first return the virtual reserves to normal: either through an actual trade or by setting correct values in setVirtualReserves(). They also set setPayRoyalties(true) for future victims and return the fee to the old values through setFeeRate().

After that, they take the profit: either through withdraw(), or through execute(), which calls baseToken.transfer(...).

Recommended Mitigation Steps

It is recommended to move the payment of royalties to the router itself, as is done in the case of the public pool: https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L114-L126

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[outdoteth]

The first issue mentioned here is so niche and unlikely that I think it can be considered invalid (an NFT collection founder purchasing NFTs from their own collection and wanting to earn their own royalties on this purchase).

The second issue of an attacker frontrunning the trade and turning off royalties is technically valid but also marginal. The choice of turning on royalties for private pools comes from the private pool owner anyway. The buyer has zero power here. That is by design. In either case, the buyer ends up paying the same amount anyway so I disagree that this is high severity.

[c4-sponsor]

outdoteth marked the issue as disagree with severity

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[GalloDaSballo]

Downgrading to QA because:

• Malicious admin is disclosed
• This requires using the router improperly, you can set tight slippage to avoid fees being lost

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-b