| Issue | Instances | |
|---|---|---|
| GAS-1 | Use selfbalance() instead of address(this).balance |
1 |
| GAS-2 | Use assembly to check for address(0) |
10 |
| GAS-3 | Cache array length outside of loop | 3 |
| GAS-4 | Use calldata instead of memory for function arguments that do not get mutated | 4 |
| GAS-5 | Use Custom Errors | 16 |
| GAS-6 | Don't initialize variables with default value | 8 |
| GAS-7 | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) |
8 |
| GAS-8 | Using private rather than public for constants, saves gas |
2 |
| GAS-9 | Use != 0 instead of > 0 for unsigned integer comparison | 4 |
| GAS-10 | internal functions not called by the contract should be removed |
2 |
Use assembly when getting a contract's balance of ETH.
You can use selfbalance() instead of address(this).balance when getting your contract's balance of ETH to save gas.
Additionally, you can use balance(address) instead of address.balance() when getting an external contract's balance of ETH.
Saves 15 gas when checking internal balance, 6 for external
Instances (1):
File: src/Pair.sol
479: ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
Saves 6 gas per instance
Instances (10):
File: src/Caviar.sol
30: require(pairs[nft][baseToken][merkleRoot] == address(0), "Pair already exists");
33: string memory baseTokenSymbol = baseToken == address(0) ? "ETH" : baseToken.tokenSymbol();
File: src/Pair.sol
74: require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");
93: if (baseToken != address(0)) {
132: if (baseToken == address(0)) {
151: require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");
166: if (baseToken == address(0)) {
198: if (baseToken == address(0)) {
465: if (merkleRoot == bytes23(0)) return;
478: return baseToken == address(0)
If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
Instances (3):
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++) {
Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.
Instances (4):
File: src/LpToken.sol
11: constructor(string memory pairSymbol)
File: src/Pair.sol
43: string memory pairSymbol,
44: string memory nftName,
45: string memory nftSymbol
Source Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.
Instances (16):
File: src/Caviar.sol
30: require(pairs[nft][baseToken][merkleRoot] == address(0), "Pair already exists");
51: require(msg.sender == pairs[nft][baseToken][merkleRoot], "Only pair can destroy itself");
File: src/Pair.sol
71: require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
74: require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");
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");
151: require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");
157: require(inputAmount <= maxInputAmount, "Slippage: amount in");
189: require(outputAmount >= minOutputAmount, "Slippage: amount out");
224: require(closeTimestamp == 0, "Wrap: closed");
343: require(caviar.owner() == msg.sender, "Close: not owner");
361: require(caviar.owner() == msg.sender, "Withdraw: not owner");
364: require(closeTimestamp != 0, "Withdraw not initiated");
367: require(block.timestamp >= closeTimestamp, "Not withdrawable yet");
470: require(isValid, "Invalid merkle proof");
Instances (8):
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++) {
File: src/lib/SafeERC20Namer.sol
12: uint256 charCount = 0;
13: for (uint256 j = 0; j < 32; j++) {
22: for (uint256 j = 0; j < charCount; j++) {
31: uint256 charCount = 0;
39: for (uint256 i = 0; i < charCount; i++) {
Saves 5 gas per loop
Instances (8):
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++) {
File: src/lib/SafeERC20Namer.sol
13: for (uint256 j = 0; j < 32; j++) {
17: charCount++;
22: for (uint256 j = 0; j < charCount; j++) {
33: for (uint256 i = 32; i < 64; i++) {
39: for (uint256 i = 0; i < charCount; i++) {
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
Instances (2):
File: src/Pair.sol
20: uint256 public constant ONE = 1e18;
21: uint256 public constant CLOSE_GRACE_PERIOD = 7 days;
Instances (4):
File: src/Pair.sol
71: require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
71: require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
169: if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
419: if (lpTokenSupply > 0) {
If the functions are required by an interface, the contract should inherit from that interface and use the override keyword
Instances (2):
File: src/lib/SafeERC20Namer.sol
76: function tokenSymbol(address token) internal view returns (string memory) {
87: function tokenName(address token) internal view returns (string memory) {
| Issue | Instances | |
|---|---|---|
| NC-1 | Missing checks for address(0) when assigning values to address state variables |
2 |
| NC-2 | Event is missing indexed fields |
8 |
| NC-3 | Constants should be defined rather than using magic numbers | 1 |
| NC-4 | Functions not used internally could be marked external | 11 |
Instances (2):
File: src/Pair.sol
47: nft = _nft;
48: baseToken = _baseToken; // use address(0) for native ETH
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.
Instances (8):
File: src/Pair.sol
30: event Add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
31: event Remove(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
32: event Buy(uint256 inputAmount, uint256 outputAmount);
33: event Sell(uint256 inputAmount, uint256 outputAmount);
34: event Wrap(uint256[] tokenIds);
35: event Unwrap(uint256[] tokenIds);
36: event Close(uint256 closeTimestamp);
37: event Withdraw(uint256 tokenId);
Instances (1):
File: src/lib/SafeERC20Namer.sol
55: return Strings.toHexString(uint160(token) >> (160 - 4 * 4));
Instances (11):
File: src/Caviar.sol
28: function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {
49: function destroy(address nft, address baseToken, bytes32 merkleRoot) public {
File: src/LpToken.sol
19: function mint(address to, uint256 amount) public onlyOwner {
26: function burn(address from, uint256 amount) public onlyOwner {
File: src/Pair.sol
275: function nftAdd(
294: function nftRemove(uint256 lpTokenAmount, uint256 minBaseTokenOutputAmount, uint256[] calldata tokenIds)
310: function nftBuy(uint256[] calldata tokenIds, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
323: function nftSell(uint256[] calldata tokenIds, uint256 minOutputAmount, bytes32[][] calldata proofs)
341: function close() public {
359: function withdraw(uint256 tokenId) public {
390: function price() public view returns (uint256) {
| Issue | Instances | |
|---|---|---|
| L-1 | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() |
1 |
[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
Instances (1):
File: src/Pair.sol
469: bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])));