Shell Protocol - 0xSmartContract's results

QA Report Issues List

• Low 01 → result require block is inconsistent with NatSpec comment
• Low 02 → libusb dependency can be a security risk
• Low 03 → There is a difference in the formula with the NatSpec comments
• Low 04 → There may be problems with the use of Arbitrum
• Non-Critical 05 → Project Upgrade and Stop Scenario should be
• Non-Critical 06 → Missing Event for initialize

[Low-01] result require block is inconsistent with NatSpec comment

result require block and Natspec comment are inconsistent, correct it if the error is caused by the comment here, if the formula is incorrect, this is an important issue

src\proteus\EvolvingProteus.sol:
  271       */
  272:     function swapGivenInputAmount(
  273:         uint256 xBalance,
  274:         uint256 yBalance,
  275:         uint256 inputAmount,
  276:         SpecifiedToken inputToken
  277:     ) external view returns (uint256 outputAmount) {
  278:         // input amount validations against the current balance
  279:         require(
  280:             inputAmount < INT_MAX && xBalance < INT_MAX && yBalance < INT_MAX
  281:         );
  282: 
  283:         _checkAmountWithBalance(
  284:             (inputToken == SpecifiedToken.X) ? xBalance : yBalance,
  285:             inputAmount
  286:         );
  287: 
  288:         int256 result = _swap(
  289:             FEE_DOWN,
  290:             int256(inputAmount),
  291:             int256(xBalance),
  292:             int256(yBalance),
  293:             inputToken
  294:         );
  295:         // amount cannot be less than 0
  296:         require(result < 0);		 // @audit => NatSpec comment and require is inconsistent
  297: 
  298:         // output amount validations against the current balance
  300:         outputAmount = uint256(-result);
  301:         _checkAmountWithBalance(
  302:             (inputToken == SpecifiedToken.X) ? yBalance : xBalance,
  303:             outputAmount
  304:         );
  305:     }

[Low-02] libusb dependency can be a security risk

When the libusb dependency is checked from the npm site, it does not provide any references, links to github or versions, creates a malicious dependency security risk, I recommend checking it and documenting what it serves

https://www.npmjs.com/package/libusb?activeTab=readme

package.json:
  26:   },
  27:   "dependencies": {
  28:     "libusb": "^0.0.0-pre"
  29:   }
  30  }

[Low-03] There is a difference in the formula with the NatSpec comments

In the NatSpec comment, x price should be less than y price. But in the formula there is also in the equation

src\proteus\EvolvingProteus.sol:
  253  
  254:         // at all times x price should be less than y price
  255:         if (py_init <= px_init) revert InvalidPrice();
  256:         if (py_final <= px_final) revert InvalidPrice();

[Low-04] There may be problems with the use of Arbitrum

According to the scope information of the project, it is stated that it be used Layer2 Arbitrum

README.md:
Which blockchains will this code be deployed to, and are considered in scope for this audit?: Arbitrum

The problem with this is that Arbitrum is Compatible with 0.8.20 and newer. Contracts compiled with these versions will result in a non-functional or potentially damaged version that does not behave as expected.

Although the project was compiled with 0.8.10, in a possible live deploy time, it may be compiled with a different compile version and this may cause it to not work on the Arbitrum network, this risk should be documented and included in the project's deploy checklist

[Non Critical-05] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[Non Critical-06] Missing Event for initialize

Use event-emit when setting the startup items in the project's constructor.

This is recommended so that Web3 Dapps and Analysis tools can monitor the set data and the project.

src\proteus\EvolvingProteus.sol:
  242      */
  243:     constructor(
  244:         int128 py_init,
  245:         int128 px_init,
  246:         int128 py_final,
  247:         int128 px_final,
  248:         uint256 duration
  249:     ) { 
  250:         // price value checks
  251:         if (py_init >= MAX_PRICE_VALUE || py_final >= MAX_PRICE_VALUE) revert MaximumAllowedPriceExceeded();
  252:         if (px_init <= MIN_PRICE_VALUE || px_final <= MIN_PRICE_VALUE) revert MinimumAllowedPriceExceeded();
  253: 
  254:         // at all times x price should be less than y price
  255:         if (py_init <= px_init) revert InvalidPrice();
  256:         if (py_final <= px_final) revert InvalidPrice();
  257: 
  258:         // max. price ratio check
  259:         if (py_init.div(py_init.sub(px_init)) > ABDKMath64x64.divu(uint(MAX_PRICE_RATIO), 1)) revert MaximumAllowedPriceRatioExceeded();
  260:         if (py_final.div(py_final.sub(px_final)) > ABDKMath64x64.divu(uint(MAX_PRICE_RATIO), 1)) revert MaximumAllowedPriceRatioExceeded();
  261: 
  262:         config = LibConfig.newConfig(py_init, px_init, py_final, px_final, duration);
  263:       }

Gas Optimization

The project is generally efficient in terms of gas optimizations, many generally accepted gas optimizations have been implemented, gas optimizations with minor effects are already mentioned in automatic finding, but gas optimizations will not be a priority considering code readability and code base size

Gas Optimization Recommendations

1- The highest gas expenditure in the project occurs when creating pool instances, if wish the architecture here was design like the singleton architecture in UniswapV4, gas savings will be close to 90%.

2- The most important gas usage of the contract subject to the audit is the ABDKMath64x64.sol library that it uses. More gas optimized prb-math may be preferred detailed gas comparisons are available on the project's github link

PRBMath

Gas estimations based on the v2.0.1 and the v3.0.0 releases.

ABDKMath64x64

Gas estimations based on the v3.0 release of ABDKMath. See my abdk-gas-estimations repo.

🛠️ Analysis - Shell Protocol

Summary

a) The approach I followed when reviewing the code

First, by examining the scope of the code, I determined my code review and analysis strategy. https://github.com/code-423n4/2023-08-shell

Accordingly, I analyzed and audited the subject in the following steps;

b) Analysis of the code base

c) Test analysis

<img width="1069" alt="image" src="https://github.com/code-423n4/2023-07-moonwell-findings/assets/104318932/d74148f3-53c5-414d-9da0-06eeb6ef1b6a">

1- ForkEvolvingProteus.t.sol :

Invariant tests can be great tools for shaking out invalid assumptions, complex edge cases, and unexpected interactions in a smart contract system. But it can also be challenging to channel the fuzzer's unconstrained chaos into a suite of meaningful, reliable tests.

Evolving Proteus Invariants test list ( ForkEvolvingProteus.t.sol ):

Invariant tests recommended to be added ( ForkEvolvingProteus.t.sol ):

d) Centralization risks

There is no centrality risk in the controlled scope.

d) Systemic risks

We can talk about a systemic risk here because there are some Layer2 special cases. If Arbitrum is compile Compatible with 0.8.20 and newer. Contracts compiled with these versions will result in a non-functional or potentially damaged version that does not behave as expected. The default behavior of the compiler will be to use the latest version which means it will compile with version 0.8.20 which will produce broken code by default.

According to the given code, this problem does not exist, but it should be taken care of while deploying. <br />

f) Competition analysis

Other projects with a similar structure to the project; 1 - dfx finance

2- Gamma

3- Osmosis

However, with its mathematical modeling, the Shell Protocol is unique.

g) Security Approach of the Project

Successful current security understandings of the project;

1- Most important security point of Shell protocol is contracts are non-custodial, meaning NOBODY (not even the core team or the Shell DAO) is able to access the funds held in any of the core contracts.

2- Proteus Pool Safeguards System ; A pool's deployer can assign a wallet the ability to freeze the pool in case of an emergency. LPs may still withdraw their tokens, but swaps and deposits are disabled. The Shell core team controls a 2/3 multi-signature wallet, the ShellMultiSig, that can freeze trading on Proteus pools assigned to it.

3- On-Chain Monitoring System ; Shell conducts on-chain security monitoring with Forta. Anyone can subscribe to updates from the Shell Forta bot. This bot tracks Shell transactions in which wrapping, unwrapping, swapping, depositing, or withdrawals occur over a threshold amount. If transactions occur with unusually high token amounts, the bot sends out an alert. https://app.forta.network/bot/0x7f9afc392329ed5a473bcf304565adf9c2588ba4bc060f7d215519005b8303e3

4- First they did the main audit from a reputable auditing organization like Trail of Bits and resolved all the security concerns in the report https://github.com/trailofbits/publications/blob/master/reviews/ShellProtocolv2.pdf

5- They manage the 2nd audit process with an innovative audit such as Code4rena, in which many auditors examine the codes.

6- They was set the $ 100,000 ImmuneFi reward https://immunefi.com/bounty/shellprotocol/

What the project should add in the understanding of Security;

1- By distributing the project to testnets, ensuring that the audits are carried out in onchain audit. (This will increase coverage)

2- After the project is published on the mainnet, there should be emergency action plans (not found in the documents)

3- No Pause Mechanism This is a chaotic situation, which can be thought of as a choice between decentralization and security. Having a pause mechanism makes sense in order not to damage user funds in case of a possible problem in the project.

4- No Upgradability There are use cases of the Upgradable pattern in defi projects using mathematical models, but it is a design and security option.

h) Other Audit Reports and Automated Findings

Automated Findings: https://github.com/code-423n4/2023-08-shell/blob/main/bot-report.md Especially Low detections in the Automated Finding Report should be taken into account;

Other Audit Reports (TrailOfBits):

Here is a summary of the issues found in the audit report with Exposure Anlysis and Category details: Audit Report TrailOfBits <img width="654" alt="image" src="https://github.com/code-423n4/2023-08-shell/assets/104318932/422c099c-7b57-4493-826e-6885fc1f20d8">

i) Gas Optimization

The project is generally efficient in terms of gas optimizations, many generally accepted gas optimizations have been implemented, gas optimizations with minor effects are already mentioned in automatic finding, but gas optimizations will not be a priority considering code readability and code base size

Gas Optimization Recommendations

1- The highest gas expenditure in the project occurs when creating pool instances, if wish the architecture here was design like the singleton architecture in UniswapV4, gas savings will be close to 90%.

2- The most important gas usage of the contract subject to the audit is the ABDKMath64x64.sol library that it uses. More gas optimized prb-math may be preferred detailed gas comparisons are available on the project's github link

PRBMath

Gas estimations based on the v2.0.1 and the v3.0.0 releases.

ABDKMath64x64

Gas estimations based on the v3.0 release of ABDKMath. See my abdk-gas-estimations repo.

j) Other recommendations

• Logic similar to singleton pattern similar to Uniswapv4 can be used in a project, this is a gas-optimized and innovative solution (Expressed for the overall project, not for the audit contract)

• The use of assembly in project codes is very low, I especially recommend using such useful and gas-optimized code patterns; https://github.com/dragonfly-xyz/useful-solidity-patterns/tree/main/patterns/assembly-tricks-1

• It is seen that the latest versions of imported important libraries such as Openzeppelin are not used in the project codes, it should be noted. https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts/

• A good model can be used to systematically assess the risk of the project, for example this modeling is recommended; https://www.notion.so/Smart-Contract-Risk-Assessment-3b067bc099ce4c31a35ef28b011b92ff#7770b3b385444779bf11e677f16e101e

• Here are my other suggestions for corrections, the details of which are in my QA Report (I didn't want to write it here again)

QA Report Issues List

• Low 01 → result require block is inconsistent with NatSpec comment
• Low 02 → libusb dependency can be a security risk
• Low 03 → There is a difference in the formula with the NatSpec comments
• Low 04 → There may be problems with the use of Arbitrum
• Non-Critical 05 → Project Upgrade and Stop Scenario should be
• Non-Critical 06 → Missing Event for initialize

k) New insights and learning from this audit

• ForkEvolvingProteus.t.sol invariant tests are extensive, quality invariant tests in a high-mathematical computational project are remarkable, the project developers did a great job
• I learned in depth the usage and details of ABDKMath64x64.sol library

Time spent:

13 hours