code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L256 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L365

Vulnerability details

Impact

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC). Fees lead to the fact that the pool actually receives less funds than expected in the contract, and the reserve configuration is changed incorrectly.

For example, in the buy() function, it is expected that netInputAmount will be received by the contract when purchasing NFT, part of which is sent as a commission to the protocol, and part as a royalty:

(netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);
...
virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);
virtualNftReserves -= uint128(weightSum);
...
ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L230-L231

After each such purchase, the NFT author and the protocol will receive the correct percentage of fees from the transfer made. However, in the pool itself, there will be less funds than what is recorded in the virtual reserve. At some point, this will lead to the fact that the NFT cannot be sold in the pool, as the pool will try to send the seller more funds than there actually are in the pool.

If the pool owner subsequently wishes to withdraw all funds from the pool, they will find that there are less funds than expected.

Same problems arise with rebaseable tokens (e. g. AMPLEFORTH).

Recommended Mitigation Steps

Regarding tokens with fees. It is recommended to calculate the received balance as it is done, for example, in yearn:

preBalance: uint256 = self.token.balanceOf(self)
loss: uint256 = Strategy(strategy).withdraw(amountNeeded)
withdrawn: uint256 = self.token.balanceOf(self) - preBalance

https://github.com/yearn/yearn-vaults/blame/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L1126-L1128

Regarding rebaseable tokens: it is recommended to clearly state in the documentation that this type of tokens is not supported.