Skip to content
This repository has been archived by the owner on Oct 29, 2023. It is now read-only.

Session 3 Numoen protocol Audit Report #12

Open
Robert-H-Leonard opened this issue Apr 22, 2023 · 2 comments
Open

Session 3 Numoen protocol Audit Report #12

Robert-H-Leonard opened this issue Apr 22, 2023 · 2 comments

Comments

@Robert-H-Leonard
Copy link
Contributor

Robert-H-Leonard commented Apr 22, 2023

Audit report for this discussion is here on the Numoen protocol: https://code4rena.com/reports/2023-01-numoen/

Please leave a comment with your analysis of an vulnerability and/or questions and thoughts your have.

@aldrinmayen
Copy link

aldrinmayen commented Apr 23, 2023

High Risk Finding H-01 precision loss in the invariant function can cause an attacket to steal funds without impacting the invariant function()
the main reason is that the current math performed in both tokens would have a different round up number causing a precision loss.

  • the main solution is allow to multiply to tokenscale before the division mulDiv function is performed to avoid loss and
    before:
    uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale;//@audit-info precison loss uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale;//@audit-info precison loss
    after:
    uint256 scale0 = FullMath.mulDiv(amount0 * token0Scale, 1e18, liquidity) ;//@audit-info change here uint256 scale1 = FullMath.mulDiv(amount1 * token1Scale, 1e18, liquidity) ;//@audit-info change here

the testAttack function is so exquisite that there is more to read and understand the prank function and idea; also it seems that attacker should know the exact amount to withdraw or swap. I am still not clear on the a+b>=c+d would return and if it is expecting true/false or a positive number and what the uint256 UpperBound represents.

@thor4
Copy link

thor4 commented Apr 30, 2023

[M-02] First liquidity provider will suffer from revert or fund loss

  • Typically when a user provides liquidity to a new pool in any protocol as the first depositor, the ratio of token0 to token1 is bounded by a function that limits how much of each token can be deposited. In Numoen it appears this is not the case. The first depositor must specify the amount of token0 and token1 themselves. If they choose incorrectly, the transaction could be reverted or funds could be lost.
  • A liquidity function is proposed that could be used to limit the amount the first depositor submits, keeping them within acceptable bounds to ensure the transaction is not reverted and funds aren't lost. However, the Numoen team originally said they didn't want to calculate it on-chain because it would be too complicated. I wonder if they considered using Chainlink Functions or Gelato Web3 Functions for this calculation..
  • The recommendation from the submitter is to calculate this function on-chain and to provide preview functions. There is no detail about the preview functions, but I assume this is for the depositor to understand the impact of their proposed transaction.
  • The Numoen team responded by saying they thought it was better for the user to pass in the amounts and that "It won’t result in loss of funds for first time depositors because they can know before time if the amount of tokens they supply match up to the amount of liquidity that they specified or not." I don't understand this statement or how it prevents the transaction from reverting or loss of funds. They also state they will look at the proposed function in more detail. It sounds like they're just dismissing the report, which seems unwise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants