Caviar contest - aviggiano's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/d42a53b569ee06254ec3b5fd17ca2e527592dfe4/src/Pair.sol#L426

Vulnerability details

Impact

Pair.addQuote calculates the amount of LP tokens received for adding a given amount of base tokens and fractional tokens. If there are no existing deposits, then initializes to Math.sqrt(baseTokenAmount * fractionalTokenAmount). The problem arises when the value of a liquidity pool share grows over time, either by accumulating trading fees or through “donations” to the liquidity pool. In theory, this could result in a situation where the value of the minimum quantity of liquidity pool shares (1e-18 pool shares) is worth so much that it becomes infeasible for small liquidity providers to provide any liquidity. (See here on Uniswap v2 whitepaper). Creating MINIMUM_LIQUIDITY tokens and sending them to address zero to lock them also makes sure that they can never be redeemed, which means the pool will never be emptied completely, and this saves us from division by zero in some places (See Ethereum.org contract walkthrough).

Proof of Concept

With time, the worth of the minimum quantity of liquidity pool share (i.e. 1**-18), becomes high. High enough that the small liquidity providers will be unable to provide liquidity.

Tools Used

Manual inspection

Recommended Mitigation Steps

To mitigate this issue, Uniswap v2 burns the first 1e-15 (0.000000000000001) pool shares that are minted (1000 times the minimum quantity of pool shares), sending them to the zero address instead of to the minter. This should be a negligible cost for almost any token pair. But it dramatically increases the cost of the above attack. In order to raise the value of a liquidity pool share to 100 USD, the attacker would need to donate 100,000 USD to the pool, which would be permanently locked up as liquidity.

diff --git a/src/Pair.sol b/src/Pair.sol
index 185d25c..73bb3ff 100644
--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -19,6 +19,7 @@ contract Pair is ERC20, ERC721TokenReceiver {
 
     uint256 public constant ONE = 1e18;
     uint256 public constant CLOSE_GRACE_PERIOD = 7 days;
+    uint256 public constant MINIMUM_LIQUIDITY = 1000;
 
     address public immutable nft;
     address public immutable baseToken; // address(0) for ETH
@@ -86,6 +87,9 @@ contract Pair is ERC20, ERC721TokenReceiver {
 
         // *** Interactions *** //
 
+        if(lpToken.totalSupply() == 0) {
+            lpToken.mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
+        }
         // mint lp tokens to sender
         lpToken.mint(msg.sender, lpTokenAmount);
 
@@ -423,7 +427,7 @@ contract Pair is ERC20, ERC721TokenReceiver {
             return Math.min(baseTokenShare, fractionalTokenShare);
         } else {
             // if there is no liquidity then init
-            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+            return Math.sqrt(baseTokenAmount * fractionalTokenAmount) - MINIMUM_LIQUIDITY;
         }
     }
 

1. SafeERC20Namer.addressToSymbol implementation does not adhere to specification, causing LP symbol/name to be incorrect in some cases

By fuzzying the existing test testItSetsSymbolsAndNames with Foundry, the test yields an error on the function SafeERC20Namer.addressToSymbol. When this function is called on the token address 0, it will incorrectly return 0x00, and not 0x0000 as expected from the specification. The issue is related to Strings.toHexString used under the hood.

Test case

diff --git a/test/Caviar/Create.t.sol b/test/Caviar/Create.t.sol
index 965a5aa..2baeb32 100644
--- a/test/Caviar/Create.t.sol
+++ b/test/Caviar/Create.t.sol
@@ -6,6 +6,29 @@ import "forge-std/console.sol";
 
 import "../shared/Fixture.t.sol";
 
+library Strings2 {
+
+    ///@dev converts bytes array to its ASCII hex string representation
+    /// TODO: Definitely more efficient way to do this by processing multiple (16?) bytes at once
+    /// but really a helper function for the tests, efficiency not key.
+    function toHexString(bytes memory input) public pure returns (string memory) {
+        require(input.length < type(uint256).max / 2 - 1);
+        bytes16 symbols = "0123456789abcdef";
+        bytes memory hex_buffer = new bytes(2 * input.length + 2);
+        hex_buffer[0] = "0";
+        hex_buffer[1] = "x";
+
+        uint pos = 2;
+        uint256 length = input.length;
+        for (uint i = 0; i < length; ++i) {
+            uint _byte = uint8(input[i]);
+            hex_buffer[pos++] = symbols[_byte >> 4];
+            hex_buffer[pos++] = symbols[_byte & 0xf];
+        }
+        return string(hex_buffer);
+    }
+}
+
 contract CreateTest is Fixture {
     event Create(address indexed nft, address indexed baseToken, bytes32 indexed merkleRoot);
 
@@ -41,6 +64,17 @@ contract CreateTest is Fixture {
         assertEq(lpToken.name(), "0xbeef:0xcafe LP token", "Should have set lp name");
     }
 
+    function testItSetsSymbolsAndNamesFuzzy(address token) public {
+        Pair pair = c.create(token, token, bytes32(0));
+
+        string[] memory cmds = new string[](3);
+        cmds[0] = "node";
+        cmds[1] = "test/testItSetsSymbolsAndNamesFuzzy.js";
+        cmds[2] = Strings2.toHexString(abi.encode(token));
+        bytes memory result = vm.ffi(cmds);
+        assertEq(pair.symbol(), string(result));
+    }
+
     function testItSavesPair() public {
         // arrange
         address nft = address(0xbeef);
diff --git a/test/testItSetsSymbolsAndNamesFuzzy.js b/test/testItSetsSymbolsAndNamesFuzzy.js
new file mode 100644
index 0000000..8dbc9c5
--- /dev/null
+++ b/test/testItSetsSymbolsAndNamesFuzzy.js
@@ -0,0 +1,6 @@
+function main(token) {
+  var address = token.substring(token.length-40, token.length)
+  return `f0x${address.substring(0, 4)}`
+}
+
+console.log(main(process.argv[2]));
\ No newline at end of file

Run with

forge test --ffi --match-test testItSetsSymbolsAndNamesFuzzy

2. Inconsistent docs on SafeERC20Namer

The documentation of SafeERC20Namer is inconsistent with its implementation, due to the modification of Uniswap's version. In their case, the function addressToSymbol would return the first 6 hex of the address string. In Caviar's case, it's the first 4 hex. Fix:

diff --git a/src/lib/SafeERC20Namer.sol b/src/lib/SafeERC20Namer.sol
index 2d8e57b..411c1b4 100644
--- a/src/lib/SafeERC20Namer.sol
+++ b/src/lib/SafeERC20Namer.sol
@@ -77,7 +77,7 @@ library SafeERC20Namer {
         // 0x95d89b41 = bytes4(keccak256("symbol()"))
         string memory symbol = callAndParseStringReturn(token, 0x95d89b41);
         if (bytes(symbol).length == 0) {
-            // fallback to 6 uppercase hex of address
+            // fallback to 4 uppercase hex of address
             return addressToSymbol(token);
         }
         return symbol;

2. Floating Pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. See SWC-103. Fix:

diff --git a/src/Caviar.sol b/src/Caviar.sol
index 674ed7b..bd7adb2 100644
--- a/src/Caviar.sol
+++ b/src/Caviar.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;
 
 import "solmate/auth/Owned.sol";
 
diff --git a/src/LpToken.sol b/src/LpToken.sol
index 345b6bd..63f8655 100644
--- a/src/LpToken.sol
+++ b/src/LpToken.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;
 
 import "solmate/auth/Owned.sol";
 import "solmate/tokens/ERC20.sol";
diff --git a/src/Pair.sol b/src/Pair.sol
index 185d25c..d99182b 100644
--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;
 
 import "solmate/tokens/ERC20.sol";
 import "solmate/tokens/ERC721.sol";
diff --git a/src/lib/SafeERC20Namer.sol b/src/lib/SafeERC20Namer.sol
index 2d8e57b..320353b 100644
--- a/src/lib/SafeERC20Namer.sol
+++ b/src/lib/SafeERC20Namer.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;
 
 import "openzeppelin/utils/Strings.sol";