Caviar contest - Bnke0x0's results

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

Findings:

2022-12-caviar/src/Pair.sol::47 => nft = _nft;
2022-12-caviar/src/Pair.sol::48 => baseToken = _baseToken; // use address(0) for native ETH
2022-12-caviar/src/Pair.sol::49 => merkleRoot = _merkleRoot;

[L02] _safeMint() should be used rather than _mint() wherever possible

Findings:

2022-12-caviar/src/LpToken.sol::20 => _mint(to, amount);
2022-12-caviar/src/Pair.sol::233 => _mint(msg.sender, fractionalTokenAmount);

[L03] Unspecific Compiler Version Pragma

Impact

Avoid floating pragmas for non-library contracts.

Findings:

2022-12-caviar/src/Caviar.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/LpToken.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/Pair.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/lib/SafeERC20Namer.sol::2 => pragma solidity ^0.8.17;

Non-Critical Issues

[N01] constants should be defined rather than using magic numbers

Findings:

2022-12-caviar/src/LpToken.sol::13 => ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)
2022-12-caviar/src/Pair.sol::20 => uint256 public constant ONE = 1e18;
2022-12-caviar/src/Pair.sol::399 => return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
2022-12-caviar/src/Pair.sol::407 => uint256 inputAmountWithFee = inputAmount * 997;
2022-12-caviar/src/Pair.sol::408 => return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
2022-12-caviar/src/lib/SafeERC20Namer.sol::55 => return Strings.toHexString(uint160(token) >> (160 - 4 * 4));

[N02] Event is missing indexed fields

Findings:

2022-12-caviar/src/Pair.sol::30 => event Add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
2022-12-caviar/src/Pair.sol::31 => event Remove(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
2022-12-caviar/src/Pair.sol::32 => event Buy(uint256 inputAmount, uint256 outputAmount);
2022-12-caviar/src/Pair.sol::33 => event Sell(uint256 inputAmount, uint256 outputAmount);

[N03] Unused file

Findings:

2022-12-caviar/src/Caviar.sol::1 => // SPDX-License-Identifier: MIT
2022-12-caviar/src/LpToken.sol::1 => // SPDX-License-Identifier: MIT
2022-12-caviar/src/Pair.sol::1 => // SPDX-License-Identifier: MIT
2022-12-caviar/src/lib/SafeERC20Namer.sol::1 => // SPDX-License-Identifier: MIT

[N04] public functions not called by the contract should be declared external instead

Impact

Contracts are allowed to override their parents’ functions and change the visibility from public to external .#### Findings:

2022-12-caviar/src/Caviar.sol::28 => function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {
2022-12-caviar/src/Caviar.sol::49 => function destroy(address nft, address baseToken, bytes32 merkleRoot) public {
2022-12-caviar/src/LpToken.sol::19 => function mint(address to, uint256 amount) public onlyOwner {
2022-12-caviar/src/LpToken.sol::26 => function burn(address from, uint256 amount) public onlyOwner {
2022-12-caviar/src/Pair.sol::147 => function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
2022-12-caviar/src/Pair.sol::182 => function sell(uint256 inputAmount, uint256 minOutputAmount) public returns (uint256 outputAmount) {
2022-12-caviar/src/Pair.sol::248 => function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {
2022-12-caviar/src/Pair.sol::280 => ) public payable returns (uint256 lpTokenAmount) {
2022-12-caviar/src/Pair.sol::310 => function nftBuy(uint256[] calldata tokenIds, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
2022-12-caviar/src/Pair.sol::341 => function close() public {
2022-12-caviar/src/Pair.sol::359 => function withdraw(uint256 tokenId) public {
2022-12-caviar/src/Pair.sol::379 => function baseTokenReserves() public view returns (uint256) {
2022-12-caviar/src/Pair.sol::383 => function fractionalTokenReserves() public view returns (uint256) {
2022-12-caviar/src/Pair.sol::390 => function price() public view returns (uint256) {
2022-12-caviar/src/Pair.sol::398 => function buyQuote(uint256 outputAmount) public view returns (uint256) {
2022-12-caviar/src/Pair.sol::406 => function sellQuote(uint256 inputAmount) public view returns (uint256) {
2022-12-caviar/src/Pair.sol::417 => function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
2022-12-caviar/src/Pair.sol::435 => function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {

[N05] Numeric values having to do with time should use time units for readability

Impact

There are units for seconds, minutes, hours, days, and weeks

Findings:

2022-12-caviar/src/Pair.sol::21 => uint256 public constant CLOSE_GRACE_PERIOD = 7 days;

[N06] NC-library/interface files should use fixed compiler versions, not floating ones

Findings:

2022-12-caviar/src/Caviar.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/LpToken.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/Pair.sol::2 => pragma solidity ^0.8.17;
2022-12-caviar/src/lib/SafeERC20Namer.sol::2 => pragma solidity ^0.8.17;

[G01] State variables only set in the constructor should be declared immutable

Impact

Avoids a Gusset (20000 gas)

Findings:

2022-12-caviar/src/Pair.sol::28 => uint256 public closeTimestamp;

[G02] <array>.length should not be looked up in every loop of a for loop

Impact

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length. Storage array length checks incur an extra Gwarmaccess (100 gas) PER-LOOP.

Findings:

2022-12-caviar/src/Pair.sol::238 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::258 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::468 => for (uint256 i = 0; i < tokenIds.length; i++) {

[G03] ++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

Findings:

2022-12-caviar/src/Pair.sol::238 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::258 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::468 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::22 => for (uint256 j = 0; j < charCount; j++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::33 => for (uint256 i = 32; i < 64; i++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::39 => for (uint256 i = 0; i < charCount; i++) {

[G04] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

Findings:

2022-12-caviar/src/Pair.sol::71 => require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

[G05] It costs more gas to initialize variables to zero than to let the default of zero be applied

Findings:

2022-12-caviar/src/Pair.sol::238 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::258 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::468 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::12 => uint256 charCount = 0;
2022-12-caviar/src/lib/SafeERC20Namer.sol::13 => for (uint256 j = 0; j < 32; j++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::22 => for (uint256 j = 0; j < charCount; j++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::31 => uint256 charCount = 0;
2022-12-caviar/src/lib/SafeERC20Namer.sol::39 => for (uint256 i = 0; i < charCount; i++) {

[G06] ++i costs less gas than i++, especially when it’s used in for loops (--i/i-- too)

Findings:

2022-12-caviar/src/Pair.sol::238 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::258 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/Pair.sol::468 => for (uint256 i = 0; i < tokenIds.length; i++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::13 => for (uint256 j = 0; j < 32; j++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::17 => charCount++;
2022-12-caviar/src/lib/SafeERC20Namer.sol::22 => for (uint256 j = 0; j < charCount; j++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::33 => for (uint256 i = 32; i < 64; i++) {
2022-12-caviar/src/lib/SafeERC20Namer.sol::35 => charCount += uint8(b[i]);
2022-12-caviar/src/lib/SafeERC20Namer.sol::39 => for (uint256 i = 0; i < charCount; i++) {

[G07] Splitting require() statements that use && Cost gas

Impact

See this issue for an example

Findings:

2022-12-caviar/src/Pair.sol::71 => require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

[G08] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

Impact

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

Findings:

2022-12-caviar/src/lib/SafeERC20Namer.sol::49 => return Strings.toHexString(uint160(token));
2022-12-caviar/src/lib/SafeERC20Namer.sol::55 => return Strings.toHexString(uint160(token) >> (160 - 4 * 4));

[G09] Duplicated require()/revert() checks should be refactored to a modifier or function

Findings:

2022-12-caviar/src/Pair.sol::74 => require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

[G10] Use custom errors rather than revert()/require() strings to save deployment gas

Findings:

2022-12-caviar/src/Caviar.sol::30 => require(pairs[nft][baseToken][merkleRoot] == address(0), "Pair already exists");
2022-12-caviar/src/Caviar.sol::51 => require(msg.sender == pairs[nft][baseToken][merkleRoot], "Only pair can destroy itself");
2022-12-caviar/src/Pair.sol::71 => require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
2022-12-caviar/src/Pair.sol::74 => require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");
2022-12-caviar/src/Pair.sol::80 => require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
2022-12-caviar/src/Pair.sol::117 => require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");
2022-12-caviar/src/Pair.sol::120 => require(fractionalTokenOutputAmount >= minFractionalTokenOutputAmount, "Slippage: fractional token out");
2022-12-caviar/src/Pair.sol::151 => require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");
2022-12-caviar/src/Pair.sol::157 => require(inputAmount <= maxInputAmount, "Slippage: amount in");
2022-12-caviar/src/Pair.sol::189 => require(outputAmount >= minOutputAmount, "Slippage: amount out");
2022-12-caviar/src/Pair.sol::224 => require(closeTimestamp == 0, "Wrap: closed");
2022-12-caviar/src/Pair.sol::343 => require(caviar.owner() == msg.sender, "Close: not owner");
2022-12-caviar/src/Pair.sol::361 => require(caviar.owner() == msg.sender, "Withdraw: not owner");
2022-12-caviar/src/Pair.sol::364 => require(closeTimestamp != 0, "Withdraw not initiated");
2022-12-caviar/src/Pair.sol::367 => require(block.timestamp >= closeTimestamp, "Not withdrawable yet");
2022-12-caviar/src/Pair.sol::470 => require(isValid, "Invalid merkle proof");

[G11] Functions guaranteed to revert when called by normal users can be marked payable

Impact

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.

Findings:

2022-12-caviar/src/LpToken.sol::19 => function mint(address to, uint256 amount) public onlyOwner {
2022-12-caviar/src/LpToken.sol::26 => function burn(address from, uint256 amount) public onlyOwner {

[G12] Empty blocks should be removed or emit something

Impact

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{...}})

Findings:

2022-12-caviar/src/Caviar.sol::21 => constructor() Owned(msg.sender) {}
2022-12-caviar/src/LpToken.sol::14 => {}