Caviar Private Pools - 0x5rings's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L733-L734

Vulnerability details

Impact

Tokens with decimals 4 or less is used would revert if being used as a based token. Unable to change a protocolFeeRate as a result.

E.g. Gemini Token as referenced in Weird ERC20 Tokens.

Code: https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L733-L734

Proof of Concept

run forge test --match-contract Quote --match-test test_changeFeeQuote_ReturnsFeeAmountForBaseTokenWithDecimals -vvvvv

when in Shibainu.sol  constructor() ERC20("Shiba Inu", "SHIB", 4) {} // change to 4 decimals p.

Foundry logs

[⠢] Compiling...
No files changed, compilation skipped

Running 1 test for test/PrivatePool/Quotes.t.sol:QuotesTest
[FAIL. Reason: Assertion failed.] test_changeFeeQuote_ReturnsFeeAmountForBaseTokenWithDecimals_maxProtocolFeeRate() (gas: 127802)
Logs:
  Error: Should have returned feeAmount
  Error: a == b not satisfied [uint]
        Left: 2478
       Right: 247800

Traces:
  [3539010] QuotesTest::setUp() 
    ├─ [3248407] → new PrivatePool@0x15cF58144EF33af1e14b5208015d11F9143E27b9
    │   └─ ← 16220 bytes of code
    ├─ [55637] PrivatePool::initialize(0x0000000000000000000000000000000000000000, Milady: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 100000000000000000000, 5000000000000000000, 1239, 200, 0x0000000000000000000000000000000000000000000000000000000000000000, true, false) 
    │   ├─ emit Initialize(baseToken: 0x0000000000000000000000000000000000000000, nft: Milady: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], virtualBaseTokenReserves: 100000000000000000000, virtualNftReserves: 5000000000000000000, changeFee: 1239, feeRate: 200, merkleRoot: 0x0000000000000000000000000000000000000000000000000000000000000000, useStolenNftOracle: true, payRoyalties: false)
    │   └─ ← ()
    ├─ [0] VM::mockCall(Factory: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], 0x6352211e00000000000000000000000015cf58144ef33af1e14b5208015d11f9143e27b9, 0x0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496) 
    │   └─ ← ()
    ├─ [46982] Milady::mint(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 0) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], id: 0)
    │   └─ ← ()
    ├─ [25082] Milady::mint(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 1) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], id: 1)
    │   └─ ← ()
    ├─ [25082] Milady::mint(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 2) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], id: 2)
    │   └─ ← ()
    ├─ [25082] Milady::mint(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 3) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], id: 3)
    │   └─ ← ()
    ├─ [25082] Milady::mint(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 4) 
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], id: 4)
    │   └─ ← ()
    └─ ← ()

  [132015] QuotesTest::test_changeFeeQuote_ReturnsFeeAmountForBaseTokenWithDecimals_maxProtocolFeeRate() 
    ├─ [0] VM::record() 
    │   └─ ← ()
    ├─ [2404] PrivatePool::baseToken() [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000
    ├─ [0] VM::accesses(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9]) 
    │   └─ ← [0x0000000000000000000000000000000000000000000000000000000000000000], []
    ├─ [0] VM::load(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
    ├─ emit WARNING_UninitedSlot(who: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], slot: 0)
    ├─ emit SlotFound(who: PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], fsig: 0xc55dae63, keysHash: 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563, slot: 0)
    ├─ [404] PrivatePool::baseToken() [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000
    ├─ [0] VM::load(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
    ├─ [0] VM::store(PrivatePool: [0x15cF58144EF33af1e14b5208015d11F9143E27b9], 0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b) 
    │   └─ ← ()
    ├─ [293] ShibaInu::decimals() [staticcall]
    │   └─ ← 4
    ├─ [0] VM::mockCall(ShibaInu: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 0x313ce5670000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f, 0x0000000000000000000000000000000000000000000000000000000000000006) 
    │   └─ ← ()
    ├─ [7521] Factory::setProtocolFeeRate(3000) 
    │   └─ ← ()
    ├─ [4989] PrivatePool::changeFeeQuote(2000000000000000000) [staticcall]
    │   ├─ [293] ShibaInu::decimals() [staticcall]
    │   │   └─ ← 4
    │   ├─ [419] Factory::protocolFeeRate() [staticcall]
    │   │   └─ ← 3000
    │   └─ ← 2478, 743
    ├─ emit log_named_string(key: Error, val: Should have returned feeAmount)
    ├─ emit log(: Error: a == b not satisfied [uint])
    ├─ emit log_named_uint(key:       Left, val: 2478)
    ├─ emit log_named_uint(key:      Right, val: 247800)
    ├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001) 
    │   └─ ← ()
    └─ ← ()

Test result: FAILED. 0 passed; 1 failed; finished in 6.49ms

Failing tests:
Encountered 1 failing test in test/PrivatePool/Quotes.t.sol:QuotesTest
[FAIL. Reason: Assertion failed.] test_changeFeeQuote_ReturnsFeeAmountForBaseTokenWithDecimals_maxProtocolFeeRate() (gas: 127802)

Tools Used

• Foundry

Recommended Mitigation Steps

Handle scenarios whereby tokens decimals are not less than a specified amount or curate an allow list of baseTokens to be used, given the amount of weirdERC20 tokens (which deviate slightly from convention standards e.g USDT or ZRX) with unexpected executions.

Judge has assessed an item in Issue #543 as 2 risk. The relevant finding follows:

changeFeeQuote assumes all base tokens will be of decimals of at > 4 decimals. However this would lead to issues whereby baseToken is of 2 decimal place.

Findings: Incorrect fee condition. PrivatePool feeRate can be set to 50% on setFeeRate and initialize

During Private Pool initialize it's assumed "check that the fee rate is less than 50%" in comments. However, fee can be set at 50%

Code: https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L172-L173

• POC

forge test --match-contract SettersTest --match-test test_setFeeRate_setsFeeRate_5000

       function test_setFeeRate_setsFeeRate_5000() public {
        // arrange
        uint16 feeRate = 5_000;

        // act
        privatePool.setFeeRate(feeRate);

        // assert - should expect aa pass even though commented "check that the fee rate is less than 50%"
        assertEq(privatePool.feeRate(), feeRate, "Should have set fee rate");
    }

Recommendation:

• if (_feeRate >= 5_000) revert FeeRateTooHigh();

---

Findings:

changeFeeQuote assumes all base tokens will be of decimals of at > 4 decimals. However this would lead to issues whereby baseToken is of 2 decimal place.

code: https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L733-L734

---

findings: Buy() - Buying NFT from the pool will become locked (always revert) if protocol amount is too high, or users can loose unexpected funds.

This is as a result of the setProtocolFeeRate exploit whereby factory.setProtocolFeeRate(type(uint16).max), given there is no cap on the fee rate.

---

Other Findings:

No real reference in the doc that fee rate must be below 50% only in the code.

• Code: https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L171-L173

• protocolFeeAmount is not referenced in the buy() natspec comments

---

A lot of missing emit events within critical functions

---

Cache storage variables

• baseToken used instead store value in cache to reduce gas requred to read a variable from storage uint256 _baseToken = baseToken.
• code: https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L648-L649

---

Floating point pragma version contracts

---