Caviar contest - 0xSmartContract's results

Summary

Low Risk Issues List

Total 7 issues

Non-Critical Issues List

Total 11 issues

Suggestions

Total 2 suggestions

[L-01] Missing ReEntrancy Guard to withdraw function

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L359-L373

Impact

Position.sol contract has no Re-Entrancy protection in withdraw function

src/Pair.sol:

 function withdraw(uint256 tokenId) public {
        // check that the sender is the caviar owner
        require(caviar.owner() == msg.sender, "Withdraw: not owner");

        // check that the close period has been set
        require(closeTimestamp != 0, "Withdraw not initiated");

        // check that the close grace period has passed
        require(block.timestamp >= closeTimestamp, "Not withdrawable yet");

        // transfer the nft to the caviar owner
        ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenId);

        emit Withdraw(tokenId);
    }

if the mint was initiated by a contract, then the contract is checked for its ability to receive ERC721 tokens. Without reentrancy guard, onERC721Received will allow an attacker controlled contract to call the mint again, which may not be desirable to some parties, like allowing minting more than allowed. https://www.paradigm.xyz/2021/08/the-dangers-of-surprising-code

Proof of Concept

If withdraw is msg.sender contract, it can do re-entrancy by overriding onERC721Received function, it doesn't seem to be a serious problem since it conforms to check-effect-interaction pattern, but this is a clear re-entry due to access to other functions and pre-emit processing. is the entracy

reentrancy.sol:
 function onERC721Received(
    address,
    address,
    uint256,
    bytes memory
  ) public virtual override returns (bytes4) {
    //...do something
    }
    return this.onERC721Received.selector;
  }

Recommended Mitigation Steps

Use Openzeppelin or Solmate Re-Entrancy pattern Here is a example of a re-entrancy guard

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

contract ReEntrancyGuard {
    bool internal locked;

    modifier noReentrant() {
        require(!locked, "No re-entrancy");
        locked = true;
        _;
        locked = false;
    }
}

[L-02] Use safeTransferOwnership instead of transferOwnership function

Context:

2 results - 2 files

src/Caviar.sol:
   4: import "solmate/auth/Owned.sol";
  12: contract Caviar is Owned {


src/LpToken.sol:
   4: import "solmate/auth/Owned.sol";
  11: contract LpToken is Owned, ERC20 {

Description: transferOwnership function is used to change Ownership from Owned.sol.

Use a 2 structure transferOwnership which is safer. safeTransferOwnership, use it is more secure due to 2-stage ownership transfer.

Recommendation: Use Ownable2Step.sol Ownable2Step.sol

[L-03] Missing Event for critical parameters init and change

Context:

src/Pair.sol:
  38  
  39:     constructor(
  40:         address _nft,
  41:         address _baseToken,
  42:         bytes32 _merkleRoot,
  43:         string memory pairSymbol,
  44:         string memory nftName,
  45:         string memory nftSymbol
  46:     ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
  47:         nft = _nft;
  48:         baseToken = _baseToken; // use address(0) for native ETH
  49:         merkleRoot = _merkleRoot;
  50:         lpToken = new LpToken(pairSymbol);
  51:         caviar = Caviar(msg.sender);
  52:     }

src/LpToken.sol:
  11  contract LpToken is Owned, ERC20 {
  12:     constructor(string memory pairSymbol)
  13:         Owned(msg.sender)
  14:         ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)
  15:     {}


src/Pair.sol:
  38  
  39:     constructor(
  40:         address _nft,
  41:         address _baseToken,
  42:         bytes32 _merkleRoot,
  43:         string memory pairSymbol,
  44:         string memory nftName,
  45:         string memory nftSymbol
  46:     ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
  47:         nft = _nft;
  48:         baseToken = _baseToken; // use address(0) for native ETH
  49:         merkleRoot = _merkleRoot;
  50:         lpToken = new LpToken(pairSymbol);
  51:         caviar = Caviar(msg.sender);
  52:     }

Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes

Recommendation: Add Event-Emit

[L-04] Loss of precision due to rounding

Add scalars so roundings are negligible

src/Pair.sol:
  390:     function price() public view returns (uint256) {
  391:         return (_baseTokenReserves() * ONE) / fractionalTokenReserves();
  392:     }

[L-05] Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists

Solmate's SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist (yet).

This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

10 results - 1 file

src/Pair.sol:
   94              // transfer base tokens in
   95:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
   96          }

  133              // if base token is native ETH then send ether to sender
  134:             msg.sender.safeTransferETH(baseTokenOutputAmount);
  135          } else {
  136              // transfer base tokens to sender
  137:             ERC20(baseToken).safeTransfer(msg.sender, baseTokenOutputAmount);
  138          }

  168              uint256 refundAmount = maxInputAmount - inputAmount;
  169:             if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
  170          } else {
  171              // transfer base tokens in
  172:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
  173          }

  199              // transfer ether out
  200:             msg.sender.safeTransferETH(outputAmount);
  201          } else {
  202              // transfer base tokens out
  203:             ERC20(baseToken).safeTransfer(msg.sender, outputAmount);
  204          }

  238          for (uint256 i = 0; i < tokenIds.length; i++) {
  239:             ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
  240          }

  258          for (uint256 i = 0; i < tokenIds.length; i++) {
  259:             ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
  260          }

  369          // transfer the nft to the caviar owner
  370:         ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenId);
  371 

Recommended Mitigation Steps

Add a contract exist control in functions;

pragma solidity >=0.8.0;

function isContract(address _addr) private returns (bool isContract) {
    isContract = _addr.code.length > 0;
}

[L-06] Should an airdrop token arrive on the pair.sol contract, it will be stuck

With the wrap() function, NFTs are transferred to the contract and in case of airdrop due to these NFTs, it will be stuck in the contract as there is no function to take these airdrop tokens from the contract.

Important NFT project owners are given airdrops, especially since the project includes NFTs such as BAYC, Moonbirds, Doodles, Azuki, there is a high probability of receiving Airdrops, but there is no function to withdraw incoming airdrop tokens, so airdrop tokens will be stuck in the contract.

A common method for airdrops is to collect airdrops with claim, so the Pair.sol contract can be considered upgradagable, adding a function to make claim.

src/Pair.sol:
  216      /// @return fractionalTokenAmount The amount of fractional tokens minted.
  217:     function wrap(uint256[] calldata tokenIds, bytes32[][] calldata proofs)
  218:         public
  219:         returns (uint256 fractionalTokenAmount)
  220:     {
  221:         // *** Checks *** //
  222: 
  223:         // check that wrapping is not closed
  224:         require(closeTimestamp == 0, "Wrap: closed");
  225: 
  226:         // check the tokens exist in the merkle root
  227:         _validateTokenIds(tokenIds, proofs);
  228: 
  229:         // *** Effects *** //
  230: 
  231:         // mint fractional tokens to sender
  232:         fractionalTokenAmount = tokenIds.length * ONE;
  233:         _mint(msg.sender, fractionalTokenAmount);
  234: 
  235:         // *** Interactions *** //
  236: 
  237:         // transfer nfts from sender
  238:         for (uint256 i = 0; i < tokenIds.length; i++) {
  239:             ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
  240:         }
  241: 
  242:         emit Wrap(tokenIds);
  243:     }

Recommended Mitigation Steps

Add this code:

 /**
  * @notice Sends ERC20 tokens trapped in contract to external address
  * @dev Onlyowner is allowed to make this function call
  * @param account is the receiving address
  * @param externalToken is the token being sent
  * @param amount is the quantity being sent
  * @return boolean value indicating whether the operation succeeded.
  *
 */
  function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) {
    IERC20(externalToken).transfer(account, amount);
    return true;
  }
}

[L-07] Add to blacklist function

NFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into liquidity. To prevent this, I recommend adding the blacklist function.

Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.

Here is the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

Recommended Mitigation Steps : Add to Blacklist function and modifier.

[N-01] Insufficient coverage

Description: The test coverage rate of the project is 97%. Testing all functions is best practice in terms of security criteria.

| File                                     | % Lines          | % Statements      | % Branches     | % Funcs        |
|------------------------------------------|------------------|-------------------|----------------|----------------|
| src/Caviar.sol                           | 100.00% (11/11)  | 100.00% (15/15)   | 100.00% (4/4)  | 100.00% (2/2)  |
| src/LpToken.sol                          | 100.00% (2/2)    | 100.00% (2/2)     | 100.00% (0/0)  | 100.00% (2/2)  |
| src/Pair.sol                             | 100.00% (88/88)  | 100.00% (107/107) | 95.24% (40/42) | 86.36% (19/22) |
| src/lib/SafeERC20Namer.sol               | 0.00% (0/38)     | 0.00% (0/53)      | 0.00% (0/12)   | 0.00% (0/7)    |

Due to its capacity, test coverage is expected to be 100%.

[N-02] NatSpec comments should be increased in contracts

Context: All Contracts

Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Recommendation: NatSpec comments should be increased in contracts

[N-03] Function writing that does not comply with the Solidity Style Guide

Context: All Contracts

Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last

[N-04] Solidity compiler optimizations can be problematic

foundry.toml:
  1: [profile.default]
  2: src = "src"
  3: out = "out"
  4: libs = ["lib"]
  5: solc = "0.8.17"
  6: optimizer_runs = 3_000

Description: Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.

Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.

Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.

Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[N-05] For modern and more readable code; update import usages

Context:

13 results - 4 files

src/Caviar.sol:
  3  
  4: import "solmate/auth/Owned.sol";
  5  
  6: import "./lib/SafeERC20Namer.sol";
  7: import "./Pair.sol";
  8  

src/LpToken.sol:
  3  
  4: import "solmate/auth/Owned.sol";
  5: import "solmate/tokens/ERC20.sol";
  6  

src/Pair.sol:
   3  
   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";
   9  
  10: import "./LpToken.sol";
  11: import "./Caviar.sol";
  12  

src/lib/SafeERC20Namer.sol:
  3  
  4: import "openzeppelin/utils/Strings.sol";
  5 

Description: Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it. This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.

Recommendation: import {contract1 , contract2} from "filename.sol";

A good example from the ArtGobblers project;

import {Owned} from "solmate/auth/Owned.sol";
import {ERC721} from "solmate/tokens/ERC721.sol";
import {LibString} from "solmate/utils/LibString.sol";
import {MerkleProofLib} from "solmate/utils/MerkleProofLib.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {ERC1155, ERC1155TokenReceiver} from "solmate/tokens/ERC1155.sol";
import {toWadUnsafe, toDaysWadUnsafe} from "solmate/utils/SignedWadMath.sol";

[N-06] Lock pragmas to specific compiler version

Description: Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally. https://swcregistry.io/docs/SWC-103

Recommendation: Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version. solidity-specific/locking-pragmas

5 results - 4 files

src/Caviar.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;
  3  

src/LpToken.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;
  3  

src/Pair.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;
  3  

src/lib/SafeERC20Namer.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;

[N-07] Use underscores for number literals

2 results - 1 file

src/Pair.sol:
  399:         return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);

  413:         return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);

Description: There is occasions where certain number have been hardcoded, either in variable or in the code itself. Large numbers can become hard to read.

Recommendation: Consider using underscores for number literals to improve its readability.

[N-08] Use of bytes.concat() instead of abi.encodePacked()

1 result - 1 file

src/Pair.sol:
  473          for (uint256 i = 0; i < tokenIds.length; i++) {
  474:             bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])));

Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled

Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked(,)

[N-09] Pragma version^0.8.17 version too recent to be trusted.

https://github.com/ethereum/solidity/blob/develop/Changelog.md 0.8.17 (2022-09-08) 0.8.16 (2022-08-08) 0.8.15 (2022-06-15) 0.8.10 (2021-11-09)

Unexpected bugs can be reported in recent versions; Risks related to recent releases Risks of complex code generation changes Risks of new language features Risks of known bugs

Use a non-legacy and more battle-tested version Use 0.8.10

[N-10] Add EIP-2981 NFT Royalty Standart Support

Consider adding EIP-2981 NFT Royalty Standard to the project

https://eips.ethereum.org/EIPS/eip-2981

Royalty (Copyright – EIP 2981):

Fixed % royalties: For example, 6% of all sales go back to artists Declining royalties: There may be a continuous decline in sales based on time or any other variable. Dynamic royalties : Varies over time or sales amount Upgradeable royalties: Allows a legal entity or NFT owner to change any copyright Incremental royalties: No royalties, for example when sold for less than $100 Managed royalties : Funds are owned by a DAO, imagine the recipient is a DAO treasury Royalties to different people : Collectors and artists can even use royalties, not specific to a particular personality

[N-11] Showing the actual values of numbers in NatSpec comments makes checking and reading code easier

src/Pair.sol:
  19  
-  20:     uint256 public constant ONE = 1e18
+  20:     uint256 public constant ONE = 1e18;  // 1_000_000_000_000_000_000
-  21:     uint256 public constant CLOSE_GRACE_PERIOD = 7 days; 
+  21:     uint256 public constant CLOSE_GRACE_PERIOD = 7 days; // 604_800 ( 7 * 24 * 60 * 60)

[S-01] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[S-02] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

Summary

Gas Optimizations List

Total 11 issues

[G-01] Use assembly to write address storage values

2 results - 1 file

src\Pair.sol:
  39     constructor(
  40         address _nft,
  41         address _baseToken,
  42         bytes32 _merkleRoot,
  43         string memory pairSymbol,
  44         string memory nftName,
  45         string memory nftSymbol
  46      ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
  47:         nft = _nft;
  48:         baseToken = _baseToken; // use address(0) for native ETH
  49          merkleRoot = _merkleRoot;
  50:         lpToken = new LpToken(pairSymbol);
  51:         caviar = Caviar(msg.sender);

Recommendation Code:

  39       constructor(
  40           address _nft,
  41           address _baseToken,
  42           bytes32 _merkleRoot,
  43           string memory pairSymbol,
  44           string memory nftName,
  45           string memory nftSymbol
  46       ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
- 47:            nft = _nft;
- 48:            baseToken = _baseToken; // use address(0) for native ETH
+                    assembly {
+ 47:                  sstore(nft.slot,_nft)
+ 48:                  sstore(baseToken,_baseToken) // use address(0) for native ETH
+                    }
  49              merkleRoot = _merkleRoot;
  50              lpToken = new LpToken(pairSymbol);
  51              caviar = Caviar(msg.sender);

[G-02] Setting the constructor to payable

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

Context:

3 results - 3 files

src\Caviar.sol:
  20  
  21:     constructor() Owned(msg.sender) {}
  22  

src\LpToken.sol:
  10  contract LpToken is Owned, ERC20 {
  11:     constructor(string memory pairSymbol)
  12          Owned(msg.sender)

src\Pair.sol:
  38  
  39:     constructor(
  40          address _nft,

Recommendation: Set the constructor to payable

[G-03] Functions guaranteed to revert_ when callled by normal users can be marked payable

If a function modifier or require such as onlyOwner-admin 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.

2 results - 1 file

src\LpToken.sol:
  19:     function mint(address to, uint256 amount) public onlyOwner {
  26:     function burn(address from, uint256 amount) public onlyOwner {

Recommendation: Functions guaranteed to revert when called by normal users can be marked payable  (for only onlyOwner functions)

[G-04] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.

2 results - 2 files

src\Caviar.sol:
  20  
  21:     constructor() Owned(msg.sender) {}
  22  

src\LpToken.sol:
  10  contract LpToken is Owned, ERC20 {
  11:     constructor(string memory pairSymbol)
  12:         Owned(msg.sender)
  13:         ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)
  14:     {}

[G-05] Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Context:  All Contracts

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Pair.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Pair.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
505fb46c  =>  add(uint256,uint256,uint256)
7ed72fda  =>  remove(uint256,uint256,uint256)
d6febde8  =>  buy(uint256,uint256)
d79875eb  =>  sell(uint256,uint256)
694ac6cf  =>  wrap(uint256[],bytes32[][])
63ac2a4c  =>  unwrap(uint256[])
68b96890  =>  nftAdd(uint256,uint256[],uint256,bytes32[][])
c4347920  =>  nftRemove(uint256,uint256,uint256[])
40c9adbe  =>  nftBuy(uint256[],uint256)
4e4d7c00  =>  nftSell(uint256[],uint256,bytes32[][])
43d726d6  =>  close()
2e1a7d4d  =>  withdraw(uint256)
203ae565  =>  baseTokenReserves()
fceacc83  =>  fractionalTokenReserves()
a035b1fe  =>  price()
168585e5  =>  buyQuote(uint256)
d7a2e4c9  =>  sellQuote(uint256)
ceb17898  =>  addQuote(uint256,uint256)
037d785b  =>  removeQuote(uint256)
cb712535  =>  _transferFrom(address,address,uint256)
eb7439df  =>  _validateTokenIds(uint256[],bytes32[][])
9f9629f8  =>  _baseTokenReserves()

[G-06] Import only what you need

Importing only what is needed is ideal for gas optimization.

13 results - 4 files

src\Caviar.sol:  
  4: import "solmate/auth/Owned.sol";
  6: import "./lib/SafeERC20Namer.sol";
  7: import "./Pair.sol";

src\LpToken.sol:
  4: import "solmate/auth/Owned.sol";
  5: import "solmate/tokens/ERC20.sol";

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";

Recommendation: import {contract1 , contract2} from "filename.sol";

[G-07] x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables

3 results - 2 files

src\Pair.sol:
  453:    balanceOf[to] += amount;
  448:    balanceOf[from] -= amount;

src\lib\SafeERC20Namer.sol:
  35:      charCount += uint8(b[i]);

src\Pair.sol:#L453
- 453:   balanceOf[to] += amount;
+ 453:   balanceOf[to] = balanceOf[to] + amount;

x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables.

[G-08] Comparison operators

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =. Using strict comparison operators hence saves gas

Context:

5 results - 1 file

src\Pair.sol:
   80:    require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
  117:    require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");
  120:    require(fractionalTokenOutputAmount >= minFractionalTokenOutputAmount, "Slippage: fractional token out");
  189:    require(outputAmount >= minOutputAmount, "Slippage: amount out");
  367:    require(block.timestamp >= closeTimestamp, "Not withdrawable yet");

  157:    require(inputAmount <= maxInputAmount, "Slippage: amount in"); 

Recommendation:  Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable.

[G-09] Use double require instead of using &&

Using double require instead of operator && can save more gas When having a require statement with 2 or more expressions needed, place the expression that cost less gas first.

Context: 

1 result - 1 file

src\Pair.sol:
  71:         require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

Recommendation Code:

src\Pair.sol#L71
- 71:   require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
+ 71:   require(baseTokenAmount > 0, "Input token amount is zero");
+ 	require(fractionalTokenAmount > 0, "Input token amount is zero");

[G-10] The solady Library's Ownable contract is significantly gas-optimized, which can be used

The project uses the onlyOwner authorization model I recommend using Solady's highly gas optimized contract.

https://github.com/Vectorized/solady/blob/main/src/auth/OwnableRoles.sol

[G-11] Separate Ether and ER20 controls into two separate internal functions

The _baseTokenReserves() function is an internal function and is used by other functions. It controls both ERC20 and Ether functions, which means higher gas.

Separate the Ether and ERC20 checks into two separate internal functions and call them in two separate functions, so no extra checks will be made.

src/Pair.sol:
      ///      xyk math is correct in the buy() and add() functions.
     function _baseTokenReserves() internal view returns (uint256) {
         return baseToken == address(0)
             ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
             : ERC20(baseToken).balanceOf(address(this));
     }