Caviar contest - IllIllI's results

Summary

Low Risk Issues

Total: 1 instances over 1 issues

Non-critical Issues

Total: 35 instances over 6 issues

Low Risk Issues

[L‑01] _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

There is 1 instance of this issue:

File: src/Pair.sol

233:          _mint(msg.sender, fractionalTokenAmount);

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L233

Non-critical Issues

[N‑01] Import declarations should import specific identifiers, rather than the whole file

Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation

There are 13 instances of this issue:

File: src/Caviar.sol

4:    import "solmate/auth/Owned.sol";

6:    import "./lib/SafeERC20Namer.sol";

7:    import "./Pair.sol";

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Caviar.sol#L4

File: src/lib/SafeERC20Namer.sol

4:    import "openzeppelin/utils/Strings.sol";

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol#L4

File: src/LpToken.sol

4:    import "solmate/auth/Owned.sol";

5:    import "solmate/tokens/ERC20.sol";

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L4

File: src/Pair.sol

4:    import "solmate/tokens/ERC20.sol";

5:    import "solmate/tokens/ERC721.sol";

6:    import "solmate/utils/MerkleProofLib.sol";

7:    import "solmate/utils/SafeTransferLib.sol";

8:    import "openzeppelin/utils/math/Math.sol";

10:   import "./LpToken.sol";

11:   import "./Caviar.sol";

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L4

[N‑02] Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact

There is 1 instance of this issue:

File: src/LpToken.sol

/// @audit IFutureYieldToken.mint()
10:   contract LpToken is Owned, ERC20 {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L10

[N‑03] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 16 instances of this issue:

File: src/lib/SafeERC20Namer.sol

/// @audit 32
11:           bytes memory bytesString = new bytes(32);

/// @audit 32
13:           for (uint256 j = 0; j < 32; j++) {

/// @audit 32
/// @audit 64
33:           for (uint256 i = 32; i < 64; i++) {

/// @audit 8
34:               charCount <<= 8;

/// @audit 64
40:               bytesStringTrimmed[i] = b[i + 64];

/// @audit 32
66:           if (data.length == 32) {

/// @audit 64
69:           } else if (data.length > 64) {

/// @audit 0x95d89b41
78:           string memory symbol = callAndParseStringReturn(token, 0x95d89b41);

/// @audit 0x06fdde03
89:           string memory name = callAndParseStringReturn(token, 0x06fdde03);

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol#L11

File: src/LpToken.sol

/// @audit 18
13:           ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L13

File: src/Pair.sol

/// @audit 18
46:       ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {

/// @audit 1000
/// @audit 997
399:          return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);

/// @audit 997
407:          uint256 inputAmountWithFee = inputAmount * 997;

/// @audit 1000
408:          return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L46

[N‑04] Non-library/interface files should use fixed compiler versions, not floating ones

There are 3 instances of this issue:

File: src/Caviar.sol

2:    pragma solidity ^0.8.17;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Caviar.sol#L2

File: src/LpToken.sol

2:    pragma solidity ^0.8.17;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L2

File: src/Pair.sol

2:    pragma solidity ^0.8.17;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L2

[N‑05] File is missing NatSpec

There is 1 instance of this issue:

File: src/lib/SafeERC20Namer.sol

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol

[N‑06] Large or complicated code bases should implement fuzzing tests

Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.

There is 1 instance of this issue:

File: src/Caviar.sol

---

Excluded findings

These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness

Summary

Low Risk Issues

Total: 2 instances over 1 issues

Non-critical Issues

Total: 22 instances over 3 issues

Low Risk Issues

[L‑01] Missing checks for address(0x0) when assigning values to address state variables

There are 2 instances of this issue:

File: src/Pair.sol

/// @audit (valid but excluded finding)
47:           nft = _nft;

/// @audit (valid but excluded finding)
48:           baseToken = _baseToken; // use address(0) for native ETH

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L47

Non-critical Issues

[N‑01] public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 11 instances of this issue:

File: src/Caviar.sol

/// @audit (valid but excluded finding)
28:       function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {

/// @audit (valid but excluded finding)
49:       function destroy(address nft, address baseToken, bytes32 merkleRoot) public {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Caviar.sol#L28

File: src/LpToken.sol

/// @audit (valid but excluded finding)
19:       function mint(address to, uint256 amount) public onlyOwner {

/// @audit (valid but excluded finding)
26:       function burn(address from, uint256 amount) public onlyOwner {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L19

File: src/Pair.sol

/// @audit (valid but excluded finding)
275       function nftAdd(
276           uint256 baseTokenAmount,
277           uint256[] calldata tokenIds,
278           uint256 minLpTokenAmount,
279           bytes32[][] calldata proofs
280:      ) public payable returns (uint256 lpTokenAmount) {

/// @audit (valid but excluded finding)
294       function nftRemove(uint256 lpTokenAmount, uint256 minBaseTokenOutputAmount, uint256[] calldata tokenIds)
295           public
296:          returns (uint256 baseTokenOutputAmount, uint256 fractionalTokenOutputAmount)

/// @audit (valid but excluded finding)
310:      function nftBuy(uint256[] calldata tokenIds, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {

/// @audit (valid but excluded finding)
323       function nftSell(uint256[] calldata tokenIds, uint256 minOutputAmount, bytes32[][] calldata proofs)
324           public
325:          returns (uint256 outputAmount)

/// @audit (valid but excluded finding)
341       function close() public {
342           // check that the sender is the caviar owner
343:          require(caviar.owner() == msg.sender, "Close: not owner");

/// @audit (valid but excluded finding)
359:      function withdraw(uint256 tokenId) public {

/// @audit (valid but excluded finding)
390:      function price() public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L275-L280

[N‑02] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 3 instances of this issue:

File: src/lib/SafeERC20Namer.sol

/// @audit 160 - (valid but excluded finding)
/// @audit 4 - (valid but excluded finding)
/// @audit 4 - (valid but excluded finding)
55:           return Strings.toHexString(uint160(token) >> (160 - 4 * 4));

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol#L55

[N‑03] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

There are 8 instances of this issue:

File: src/Pair.sol

/// @audit (valid but excluded finding)
30:       event Add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);

/// @audit (valid but excluded finding)
31:       event Remove(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);

/// @audit (valid but excluded finding)
32:       event Buy(uint256 inputAmount, uint256 outputAmount);

/// @audit (valid but excluded finding)
33:       event Sell(uint256 inputAmount, uint256 outputAmount);

/// @audit (valid but excluded finding)
34:       event Wrap(uint256[] tokenIds);

/// @audit (valid but excluded finding)
35:       event Unwrap(uint256[] tokenIds);

/// @audit (valid but excluded finding)
36:       event Close(uint256 closeTimestamp);

/// @audit (valid but excluded finding)
37:       event Withdraw(uint256 tokenId);

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L30

Summary

Gas Optimizations

Total: 15 instances over 7 issues with 614 gas saved

Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.

Gas Optimizations

[G‑01] internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There is 1 instance of this issue:

File: src/Pair.sol

463:      function _validateTokenIds(uint256[] calldata tokenIds, bytes32[][] calldata proofs) internal view {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L463

[G‑02] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There is 1 instance of this issue:

File: src/Pair.sol

/// @audit require() on line 157
168:              uint256 refundAmount = maxInputAmount - inputAmount;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L168

[G‑03] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 7 instances of this issue:

File: src/lib/SafeERC20Namer.sol

13:           for (uint256 j = 0; j < 32; j++) {

22:           for (uint256 j = 0; j < charCount; j++) {

33:           for (uint256 i = 32; i < 64; i++) {

39:           for (uint256 i = 0; i < charCount; i++) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol#L13

File: src/Pair.sol

238:          for (uint256 i = 0; i < tokenIds.length; i++) {

258:          for (uint256 i = 0; i < tokenIds.length; i++) {

468:          for (uint256 i = 0; i < tokenIds.length; i++) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L238

[G‑04] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There are 2 instances of this issue:

File: src/Caviar.sol

/// @audit create(), destroy()
12:   contract Caviar is Owned {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Caviar.sol#L12

File: src/Pair.sol

/// @audit add(), remove(), buy(), sell(), wrap(), unwrap(), nftAdd(), nftRemove(), nftBuy(), nftSell(), close(), baseTokenReserves(), fractionalTokenReserves(), price(), buyQuote(), sellQuote(), addQuote(), removeQuote()
16:   contract Pair is ERC20, ERC721TokenReceiver {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L16

[G‑05] Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas

There is 1 instance of this issue:

File: src/Pair.sol

71:           require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L71

[G‑06] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There is 1 instance of this issue:

File: src/Pair.sol

25:       bytes32 public immutable merkleRoot;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L25

[G‑07] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 2 instances of this issue:

File: src/LpToken.sol

19:       function mint(address to, uint256 amount) public onlyOwner {

26:       function burn(address from, uint256 amount) public onlyOwner {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/LpToken.sol#L19

---

Excluded findings

These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness

Summary

Gas Optimizations

Total: 29 instances over 4 issues with 49 gas saved

Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.

Gas Optimizations

[G‑01] <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 3 instances of this issue:

File: src/Pair.sol

/// @audit (valid but excluded finding)
238:          for (uint256 i = 0; i < tokenIds.length; i++) {

/// @audit (valid but excluded finding)
258:          for (uint256 i = 0; i < tokenIds.length; i++) {

/// @audit (valid but excluded finding)
468:          for (uint256 i = 0; i < tokenIds.length; i++) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L238

[G‑02] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 8 instances of this issue:

File: src/lib/SafeERC20Namer.sol

/// @audit (valid but excluded finding)
13:           for (uint256 j = 0; j < 32; j++) {

/// @audit (valid but excluded finding)
17:                   charCount++;

/// @audit (valid but excluded finding)
22:           for (uint256 j = 0; j < charCount; j++) {

/// @audit (valid but excluded finding)
33:           for (uint256 i = 32; i < 64; i++) {

/// @audit (valid but excluded finding)
39:           for (uint256 i = 0; i < charCount; i++) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/lib/SafeERC20Namer.sol#L13

File: src/Pair.sol

/// @audit (valid but excluded finding)
238:          for (uint256 i = 0; i < tokenIds.length; i++) {

/// @audit (valid but excluded finding)
258:          for (uint256 i = 0; i < tokenIds.length; i++) {

/// @audit (valid but excluded finding)
468:          for (uint256 i = 0; i < tokenIds.length; i++) {

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L238

[G‑03] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 2 instances of this issue:

File: src/Pair.sol

/// @audit (valid but excluded finding)
20:       uint256 public constant ONE = 1e18;

/// @audit (valid but excluded finding)
21:       uint256 public constant CLOSE_GRACE_PERIOD = 7 days;

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L20

[G‑04] Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 16 instances of this issue:

File: src/Caviar.sol

/// @audit (valid but excluded finding)
30:           require(pairs[nft][baseToken][merkleRoot] == address(0), "Pair already exists");

/// @audit (valid but excluded finding)
51:           require(msg.sender == pairs[nft][baseToken][merkleRoot], "Only pair can destroy itself");

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Caviar.sol#L30

File: src/Pair.sol

/// @audit (valid but excluded finding)
71:           require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

/// @audit (valid but excluded finding)
74:           require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

/// @audit (valid but excluded finding)
80:           require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

/// @audit (valid but excluded finding)
117:          require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");

/// @audit (valid but excluded finding)
120:          require(fractionalTokenOutputAmount >= minFractionalTokenOutputAmount, "Slippage: fractional token out");

/// @audit (valid but excluded finding)
151:          require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");

/// @audit (valid but excluded finding)
157:          require(inputAmount <= maxInputAmount, "Slippage: amount in");

/// @audit (valid but excluded finding)
189:          require(outputAmount >= minOutputAmount, "Slippage: amount out");

/// @audit (valid but excluded finding)
224:          require(closeTimestamp == 0, "Wrap: closed");

/// @audit (valid but excluded finding)
343:          require(caviar.owner() == msg.sender, "Close: not owner");

/// @audit (valid but excluded finding)
361:          require(caviar.owner() == msg.sender, "Withdraw: not owner");

/// @audit (valid but excluded finding)
364:          require(closeTimestamp != 0, "Withdraw not initiated");

/// @audit (valid but excluded finding)
367:          require(block.timestamp >= closeTimestamp, "Not withdrawable yet");

/// @audit (valid but excluded finding)
470:              require(isValid, "Invalid merkle proof");

https://github.com/code-423n4/2022-12-caviar/blob/b4b32c049ae71178e3bf5125a3b87a4eebb866f3/src/Pair.sol#L71