Caviar contest - Rolezn's results

Summary<a name="Summary">

Low Risk Issues

Total: 7 contexts over 3 issues

Non-critical Issues

Total: 15 contexts over 6 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Use _safeMint instead of _mint

According to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible. https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-

<ins>Proof Of Concept</ins>

20: _mint(to, amount);

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L20

233: _mint(msg.sender, fractionalTokenAmount);

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

<ins>Recommended Mitigation Steps</ins>

Use _safeMint whenever possible instead of _mint

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L8

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L4

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> Missing parameter validation in constructor

Some parameters of constructors are not checked for invalid values.

<ins>Proof Of Concept</ins>

40: address _nft
41: address _baseToken
42: bytes32 _merkleRoot,

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L40-L42

<ins>Recommended Mitigation Steps</ins>

Validate the parameters.

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

<ins>Proof Of Concept</ins>

21: constructor() Owned(msg.sender)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L21

11: constructor(string memory pairSymbol)
        Owned(msg.sender)
        ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L11

39: constructor(
        address _nft,
        address _baseToken,
        bytes32 _merkleRoot,
        string memory pairSymbol,
        string memory nftName,
        string memory nftSymbol
    ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L39

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Avoid Floating Pragmas: The Version Should Be Locked

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

<ins>Proof Of Concept</ins>

Found usage of floating pragmas ^0.8.17 of Solidity in [Caviar.sol]

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

Found usage of floating pragmas ^0.8.17 of Solidity in [LpToken.sol]

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

Found usage of floating pragmas ^0.8.17 of Solidity in [Pair.sol]

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

Found usage of floating pragmas ^0.8.17 of Solidity in [SafeERC20Namer.sol]

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L2

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Use of Block.Timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116

<ins>Proof Of Concept</ins>

367: require(block.timestamp >= closeTimestamp, "Not withdrawable yet");

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L367

<ins>Recommended Mitigation Steps</ins>

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Non-usage of specific imports

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

A good example:

import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
import {SafeCastLib} from "solmate/utils/SafeCastLib.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";
import {IProducer} from "src/interfaces/IProducer.sol";
import {GlobalState, UserState} from "src/Common.sol";

<ins>Proof Of Concept</ins>

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L6-L7

10: import "./LpToken.sol";
11: import "./Caviar.sol";

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L10-L11

<ins>Recommended Mitigation Steps</ins>

Use specific imports syntax per solidity docs recommendation.

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

<ins>Proof Of Concept</ins>

469: bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L469

<ins>Recommended Mitigation Steps</ins>

Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> No need to initialize uints to zero

There is no need to initialize uint variables to zero as their default value is 0

<ins>Proof Of Concept</ins>

12: uint256 charCount = 0;

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L12

31: uint256 charCount = 0;

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L31

Summary<a name="Summary">

Gas Optimizations

Total: 48 contexts over 12 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables

<ins>Proof Of Concept</ins>

448: balanceOf[from] -= amount;
453: balanceOf[to] += amount;

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L448

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L453

35: charCount += uint8(b[i]);

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L35

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> ++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

<ins>Proof Of Concept</ins>

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

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

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L258

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L468

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L13

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L22

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L33

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L39

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas

<ins>Proof Of Concept</ins>

51: require(msg.sender == pairs[nft][baseToken][merkleRoot], "Only pair can destroy itself");

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L51

80: require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L80

117: require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L117

120: require(fractionalTokenOutputAmount >= minFractionalTokenOutputAmount, "Slippage: fractional token out");

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L120

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> Splitting require() statements that use && saves gas

Instead of using operator && on a single require. Using a two require can save more gas.

i.e. for require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); use:

	require(version == 1);
	require(_bytecodeHash[1] == bytes1(0));

<ins>Proof Of Concept</ins>

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

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

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>

function create(address nft, address baseToken, bytes32 merkleRoot) public returns (Pair pair) {

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

function destroy(address nft, address baseToken, bytes32 merkleRoot) public {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L49

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

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

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L26

function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L147

function sell(uint256 inputAmount, uint256 minOutputAmount) public returns (uint256 outputAmount) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L182

function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L248

function nftAdd(
        uint256 baseTokenAmount,
        uint256[] calldata tokenIds,
        uint256 minLpTokenAmount,
        bytes32[][] calldata proofs
    ) public payable returns (uint256 lpTokenAmount) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L275

function nftBuy(uint256[] calldata tokenIds, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L310

function close() public {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L341

function withdraw(uint256 tokenId) public {

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

function baseTokenReserves() public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L379

function fractionalTokenReserves() public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L383

function price() public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L390

function buyQuote(uint256 outputAmount) public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L398

function sellQuote(uint256 inputAmount) public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L406

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L417

function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L435

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> 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.

See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>

<ins>Proof Of Concept</ins>

File: .\Projects\caviar202212\2022-12-caviar\src\Caviar.sol

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol

File: .\Projects\caviar202212\2022-12-caviar\src\LpToken.sol

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol

File: .\Projects\caviar202212\2022-12-caviar\src\Pair.sol

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol

<ins>Recommended Mitigation Steps</ins>

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 Gauge.sol contract will be the most used; A lower method ID may be given.

<a href="#Summary">[GAS‑7]</a><a name="GAS&#x2011;7"> Using fixed bytes is cheaper than using string

As a rule of thumb, use bytes for arbitrary-length raw byte data and string for arbitrary-length string (UTF-8) data. If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

<ins>Proof Of Concept</ins>

33: string memory baseTokenSymbol = baseToken == address(0) ? "ETH" : baseToken.tokenSymbol();

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L33

36: string memory pairSymbol = string.concat(nftSymbol, ":", baseTokenSymbol);

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L36

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Superfluous event fields

block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.

<ins>Proof Of Concept</ins>

36: event Close(uint256 closeTimestamp);

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L36

<a href="#Summary">[GAS‑9]</a><a name="GAS&#x2011;9"> internal functions only called once can be inlined to save gas

<ins>Proof Of Concept</ins>

463: function _validateTokenIds

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

76: function tokenSymbol

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L76

87: function tokenName

https://github.com/code-423n4/2022-12-caviar/tree/main/src/lib\SafeERC20Namer.sol#L87

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Setting the constructor to payable

Saves ~13 gas per instance

<ins>Proof Of Concept</ins>

21: constructor() Owned(msg.sender)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Caviar.sol#L21

11: constructor(string memory pairSymbol)
        Owned(msg.sender)
        ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L11

39: constructor(
        address _nft,
        address _baseToken,
        bytes32 _merkleRoot,
        string memory pairSymbol,
        string memory nftName,
        string memory nftSymbol
    ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18)

https://github.com/code-423n4/2022-12-caviar/tree/main/src/Pair.sol#L39

<a href="#Summary">[GAS‑11]</a><a name="GAS&#x2011;11"> Functions guaranteed to revert when called by normal users can be marked payable

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

<ins>Proof Of Concept</ins>

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

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

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

https://github.com/code-423n4/2022-12-caviar/tree/main/src/LpToken.sol#L26

<ins>Recommended Mitigation Steps</ins>

Functions guaranteed to revert when called by normal users can be marked payable.

<a href="#Summary">[GAS‑12]</a><a name="GAS&#x2011;12"> Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

<ins>Proof Of Concept</ins>

168: uint256 refundAmount = maxInputAmount - inputAmount;

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