Caviar contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L99 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L275-L286 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L417-L428

Vulnerability details

Impact

Usually on the UI page, whenever a base token amount is entered, the app will calculate the corresponding amount for fractional token amount and vice versa with minLpTokenAmount displayed along based off xy = k. However, some stakers preferring to interact with add() and nftAdd() via the etherscan or from their custom contract(s) would be at risk entering the wrong function arguments that could lead to undesirable result.

Proof of Concept

File: Pair.sol#L417-L428

    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

As can be seen in the code block above, the function logic does not care about the parameter input amounts. All it would do is taking the minimum of baseTokenShare and fractionalTokenShare as long as this is not the first stake to the pool.

Here is a possible scenario with the current reserves:

1. Alice was the first staker providing 1000 USDC and 10 NFT via `nftAdd()`:

// (1000 * 10) ** (1/2)
lpToken.totalSupply() == 100e18

2. Bob became the second staker by accidentally providing 100 USDC (instead of 1000 USDC) and 10 NFT via `nftAdd()`. 
Worse, he also erroneously entered 0 for `minLpTokenAmount`:

// (100 * 100) / 1000 = 10
uint256 baseTokenShare == 10e18
// (10 * 100) / 10 = 100
uint256 fractionalTokenShare == 100e18

3. Bob received an lp amount of 10. He ended up oversupplying the fractional token and not refunded the difference. 
He would also likely not be able to unwrap all of his NFTs with a tenth of the supposed lp amounts received had he supplied 1000 USDC.

Tools Used

Manual inspection

Recommended Mitigation Steps

Although one can argue that it is users' responsibilities to be careful about their input arguments, I believe a value added service from the protocol could only add to the platform in a positive manner. Consider adding an early require check in add() such that xy = k is satisfied prior to allowing the function logic to proceed further. The suggested codes could be as follows based off the reasoning that operand0 == operand1, i.e. xdy = ydx:

        uint256 operand0 = baseTokenReserves() * fractionalTokenAmount;
        uint256 operand1 = fractionalTokenReserves() * baseTokenAmount;

        // 5 % tolerance to cater for irrational numbers with non-repeating decimals
        if (baseTokenReserves() > 0 || fractionalTokenReserves() > 0) {
            require(
                (Math.min(operand0, operand1) * 100) / Math.max(operand0, operand1) >= 95,
                "x / y != dx / dy not satisfied."
            );
        }

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are the instances with missing NatSpec:

File: Caviar.sol#L15-L16

    event Create(address indexed nft, address indexed baseToken, bytes32 indexed merkleRoot);
    event Destroy(address indexed nft, address indexed baseToken, bytes32 indexed merkleRoot);

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L20-L52 https://github.com/code-423n4/2022-12-caviar/blob/main/src/LpToken.sol#L11-L14

delete implication in destroy() of Caviar.sol

As documented in the link below:

https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=delete#delete

delete is actually not really deletion at all - it's only assigning a default value back to variables. It does not make difference whether the variable is in memory or in storage. Specifically, it has no effect on whole mappings (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The code line below is merely resetting the selected pair to zero address and not destroying anything in essence. Neither will it have any effect on the operation of Pair.sol:

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Caviar.sol#L53-L54

        // delete the pair
        delete pairs[nft][baseToken][merkleRoot];

Rebase/fee-on-transfer tokens

Since rebase and fee-on-transfer tokens are not supported by the AMM, there should be a whitelist logic in create() of Caviar.sol preventing the deployment of any pair that would have baseToken associated with them. Otherwise, there will not be any guarantee of not using these tokens to break the swap curve and liquidity maths.

Stuck tokens/nfts

There is simply no contracts that could prevent this from happening. Consider implementing access controlled withdraw functions to cater for these situations that could be appear as follows:

    function withdrawOtherToken(ERC20 otherToken) external {
        require(caviar.owner() == msg.sender, "withdrawOtherToken: not owner");
        require(address(otherToken) != baseToken, "INVALID_TOKEN"); 
        uint256 bal = otherToken.balanceOf(address(this));
        require(bal > 0, "INSUFFICIENT_BALANCE");
        otherToken.safeTransfer(msg.sender, bal);
    } 

    function withdrawOtherNft(ERC721 _otherNft, uint256 tokenId) external {
        require(caviar.owner() == msg.sender, "withdrawOtherNft: not owner");
        require(address(otherNft) != nft, "INVALID_NFT"); 
        otherNft.safeTransferFrom(address(this), msg.sender, tokenId);
    } 

Note: Base tokens and fractional tokens accidentally sent in to Pair.sol would only benefit all existing liquidity providers as the tokens will correspondingly be reflected in baseTokenReserves() and fractionalTokenReserves().

Use bytes32 instead of bytes23

bytes23 was used in the code line below instead of bytes32. Although merkleRoot of bytes32(0) might skip the first if block of _validateTokenIds(), it will eventually revert in the for loop that follows. Consider refactoring the affected code line as follows so it could end the function logic earlier to avoid unnecessary wastage of gas:

File: Pair.sol#L465

-        if (merkleRoot == bytes23(0)) return;
+        if (merkleRoot == bytes32(0)) return;

Unspecific compiler version pragma

For most source-units the compiler version pragma is very unspecific ^0.8.17. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored as follows:

File: Caviar.sol#L4-L7

import {Owned} from "solmate/auth/Owned.sol";

import {SafeERC20Namer} from "./lib/SafeERC20Namer.sol";
import {Pair.sol} from "./Pair.sol";

Non-fungible nature of NFTs

It is difficult to maintain a collection of NFTs with the same values considering at some point in the future, some of them are going to be worth more than the others. When this happened, traders would race into cherry picking the tokenIds desired when attempting to call nftBuy() and/or unwrap().

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: Pair.sol#L477-L481

-    function _baseTokenReserves() internal view returns (uint256) {
+    function _baseTokenReserves() internal view returns (uint256 baseTokenReserves) {
-        return baseToken == address(0)
+        baseTokenReserves baseToken == address(0)
            ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
            : ERC20(baseToken).balanceOf(address(this));
    }

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For example, the += instance below may be refactored as follows:

File: Pair.sol#L453

-            balanceOf[to] += amount;
+            balanceOf[to] = balanceOf[to] + amount;

Similarly, the -= instance below may be refactored as follows:

File: Pair.sol#L448

-        balanceOf[from] -= amount;
+        balanceOf[from] = balanceOf[from] - amount;

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ++i ;} to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

File: Pair.sol#L468-L471

-        for (uint256 i = 0; i < tokenIds.length; i++) {
+        for (uint256 i = 0; i < tokenIds.length;) {
            bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])));
            require(isValid, "Invalid merkle proof");

+            unchecked {
+                ++i;
+            }
        }

State Variables Repeatedly Read Should be Cached

SLOADs cost 100 gas each after the 1st one whereas MLOADs/MSTOREs only incur 3 gas each. As such, storage values read multiple times should be cached in the stack memory the first time (costing only 1 SLOAD) and then re-read from this cache to avoid multiple SLOADs.

For instance, closeTimestamp in the code block below could be cached as follows:

File: Pair.sol#L364-L367

+        uint256 _closeTimestamp = closeTimestamp; // SLOAD 1

-        require(closeTimestamp != 0, "Withdraw not initiated");
+        require(_closeTimestamp != 0, "Withdraw not initiated"); // MLOAD 1

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

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: Pair.sol#L80

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

Similarly, consider replacing <= with the strict counterpart < in the following inequality instance, as an example:

File: Pair.sol#L157

-        require(inputAmount <= maxInputAmount, "Slippage: amount in");
+        require(inputAmount < maxInputAmount + 1, "Slippage: amount in");

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here is an instance entailed:

File: Pair.sol#L71

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

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the code line below may be refactored as follows:

File: Pair.sol#L341

-    function close() public {
+    function close() public payable {

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

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

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
  version: "0.8.16",
  settings: {
    optimizer: {
      enabled: true,
      runs: 1000,
    },
  },
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, 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. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.