Platform: Code4rena
Start Date: 07/04/2023
Pot Size: $47,000 USDC
Total HM: 20
Participants: 120
Period: 6 days
Judge: GalloDaSballo
Total Solo HM: 4
Id: 230
League: ETH
Rank: 3/120
Findings: 3
Award: $2,509.32
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: rvierdiiev
2437.5794 USDC - $2,437.58
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654
Flashloan fee is not distributed to the factory
When user takes a flashloan, then he pays a fee to the PrivatePool. The problem is that the whole fee amount is sent to PrivatePool and factory receives nothing.
However, all other function of contract send some part of fees to the factory.
For example, change function, which is similar to the flashloan as it doesn't change virtual nft and balance reserves. This function calculates pool and protocol fees.
But in case of flashloan, only pool receives fees.
VsCode
Send some part of flashloan fee to the factory.
#0 - c4-pre-sort
2023-04-20T17:49:58Z
0xSorryNotSorry marked the issue as primary issue
#1 - c4-sponsor
2023-04-22T13:22:43Z
outdoteth marked the issue as sponsor confirmed
#2 - outdoteth
2023-04-24T11:25:47Z
Fixed in: https://github.com/outdoteth/caviar-private-pools/pull/8
Proposed fix is to add a method that returns the protocol fee and flash fee. And then have the flash fee function sum the two outputs:
function flashFeeAndProtocolFee() public view returns (uint256 feeAmount, uint256 protocolFeeAmount) { // multiply the changeFee to get the fee per NFT (4 decimals of accuracy) uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4; feeAmount = changeFee * 10 ** exponent; protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000; } function flashFee(address, uint256) public view returns (uint256) { (uint256 feeAmount, uint256 protocolFeeAmount) = flashFeeAndProtocolFee(); return feeAmount + protocolFeeAmount; }
and then add the protocol payment in the flashLoan method:
// -- snip -- // if (baseToken != address(0)) { // transfer the fee from the borrower ERC20(baseToken).safeTransferFrom(msg.sender, address(this), flashFee); // transfer the protocol fee to the factory ERC20(baseToken).safeTransferFrom(msg.sender, factory, protocolFee); } else { // transfer the protocol fee to the factory factory.safeTransferETH(protocolFee); }
#3 - GalloDaSballo
2023-04-30T08:45:13Z
@outdoteth Can you please confirm if you originally intended to have the protocol charge a fee for Flashloans?
#4 - outdoteth
2023-05-01T19:50:21Z
It was an oversight that we did not charge fees on flash loans. It's implied that it should be paid though since protocol fees are charged everywhere else a user makes a transaction.
#5 - c4-judge
2023-05-02T07:30:04Z
GalloDaSballo marked the issue as selected for report
#6 - GalloDaSballo
2023-05-02T07:30:35Z
The Warden has found an inconsistency as to how fees are paid, after confirming with the Sponsor I agree with Medium Severity
🌟 Selected for report: AkshaySrivastav
Also found by: 0xRobocop, Koolex, adriro, bin2chen, bshramin, chaduke, cryptonue, rbserver, rvierdiiev, saian, sashik_eth, tallo
40.7364 USDC - $40.74
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289
PrivatePool.buy will not take more fees from sender in case if royalty receiver is 0, but fee is not
Using PrivatePool.buy function user can buy several nft from the pool.
When sale price is calculated, then in case of payRoyalties, amount that should be paid as royalty is calculated and added to the total amount. Pls, note that it's not checked that royalty recipient is not 0.
Later, royalties amount is added to netInputAmount and then in case of eth payment, amount that is msg.value - netInputAmount is refunded to sender.
Then later, royalty payments are done. In case if royaltyFee > 0 && recipient != address(0) then it's sent to recipient.
That means that in case if any call to (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice) will return royaltyFee > 0 && recipient == 0, then this royaltyFee value will be took from sender, but will not be sent out of contract.
As result, sender overpays fees in this case.
Also another thing that can happen there when royaltyFee > 0 && recipient == 0 is that this check can revert in case if user provided less than netInputAmount, but it still can be enough to pay for nft.
VsCode
In case if royalty fee recipient is address 0, then do not include it to netInputAmount.
#0 - c4-pre-sort
2023-04-20T16:50:04Z
0xSorryNotSorry marked the issue as duplicate of #596
#1 - c4-judge
2023-05-01T07:16:07Z
GalloDaSballo marked the issue as satisfactory
🌟 Selected for report: AkshaySrivastav
Also found by: 0xRobocop, Koolex, adriro, bin2chen, bshramin, chaduke, cryptonue, rbserver, rvierdiiev, saian, sashik_eth, tallo
40.7364 USDC - $40.74
https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L344 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L190 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L123
EthRouter.buy and EthRouter.sell doesn't check that royalty recipient is not 0
EthRouter.buy and EthRouter.sell function sends royalty to the receiver in case of public pool. But they don't check if recipient is 0 and send royalty to 0 address in that case.
Pls, note that PrivatePool.buy, checks that royalty recipient is not 0 and avoids situations when royalwy fee is not 0 and recipient is 0.
VsCode
In case if royalty recipient is 0, then do not send royalty.
#0 - c4-pre-sort
2023-04-20T16:50:00Z
0xSorryNotSorry marked the issue as duplicate of #596
#1 - c4-judge
2023-05-01T07:16:07Z
GalloDaSballo marked the issue as satisfactory