Shell Protocol - 0xmystery's results

Unreachable if block in function _getUtility

In function _getUtility:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L720-L721

        if(a < 0 && b < 0) utility = (r0 > r1) ? r1 : r0;
        else utility = (r0 > r1) ? r0 : r1;

a and b are the output of a square root function below. Hence they are always greater than or equal to zero, making the if block unreachable at all times.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L111-L125

    /**
       @notice Calculates the a variable in the curve eq which is basically a sq. root of the inverse of y instantaneous price
       @param self config instance
    */
    function a(Config storage self) public view returns (int128) {
        return (p_max(self).inv()).sqrt();
    }

    /**
       @notice Calculates the b variable in the curve eq which is basically a sq. root of the inverse of x instantaneous price
       @param self config instance
    */
    function b(Config storage self) public view returns (int128) {
        return p_min(self).sqrt();
    }

Use uint256 instead of uint

In favor of explicitness, consider replacing the following instances of uint with uint256:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L259-L260

        if (py_init.div(py_init.sub(px_init)) > ABDKMath64x64.divu(uint(MAX_PRICE_RATIO), 1)) revert MaximumAllowedPriceRatioExceeded();
        if (py_final.div(py_final.sub(px_final)) > ABDKMath64x64.divu(uint(MAX_PRICE_RATIO), 1)) revert MaximumAllowedPriceRatioExceeded();

Typo errors

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L736 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L768

     @ audit is was ==> is
     *  without caring about which particular function is was called.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L39

           @ audit a ==> A
           Max transaction amount: a transaction amount cannot be too large relative to the size of the reserves in the pool.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L41

           @ audit the ==> The
           Max reserve ratio: the ratio between the two reserves cannot go above a certain ratio. Any transaction that results in the reserves going beyond this ratio will fall.

Skip r1 if disc is zero

When the discriminant is zero, there can only be 1 unique solution. Here's a suggested code refactoring:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L716-L718

        int256 disc = int256(Math.sqrt(uint256((bQuad**2 - (aQuad.muli(cQuad)*4)))));
        int256 r0 = (-bQuad*MULTIPLIER + disc*MULTIPLIER) / aQuad.mul(two).muli(MULTIPLIER);
-        int256 r1 = (-bQuad*MULTIPLIER - disc*MULTIPLIER) / aQuad.mul(two).muli(MULTIPLIER);
+        if (disc != 0) int256 r1 = (-bQuad*MULTIPLIER - disc*MULTIPLIER) / aQuad.mul(two).muli(MULTIPLIER);

Chain reorganization attack

As denoted in the Moralis academy article:

"... If a node receives a new chain that’s longer than its current active chain of blocks, it will do a chain reorg to adopt the new chain, regardless of how long it is."

Depending on the outcome, if it ended up placing the transaction earlier than anticipated, many of the system function calls could backfire. With slippage protection, this should not affect a swap or an LP token minting/burning. But it could interfere with other function calls involving governance, Ducth auction etc that the protocol might need to be aware of.

(Note: On Ethereum this is unlikely but this is meant for contracts going to be deployed on any compatible EVM chain many of which like Polygon, Optimism, Arbitrum are frequently reorganized.)

Use decimal instead of hexadecimal

Where possible, adopt the decimal (in lieu of hexadecimal) number system for readability.

For instance, the constant assignment below may be refactored like it has been done so on constant MAX_PRICE_VALUE:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L157

-    int128 constant MAX_M = 0x5f5e1000000000000000000;
+    int128 constant MAX_M = 1844674407370955161600000000;

Similarly, the constant assignment below may be refactored like it has been done so on constant MIN_PRICE_VALUE

-    int128 constant MIN_M = 0x00000000000002af31dc461;
+    int128 constant MIN_M = 184467440737;

Comment mismatch on a(t) and b(t)

It is p_max(t) and p_min(t) (in stead of a(t) and b(t)), and py_init, py_final, px_init and px_final (in stead of a_init, a_final, b_init and b_final) pertaining to the comment below.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L29-L35

     The parameters "a" and "b" are calculated from the price. a = 1/sqrt(y_axis_price) and b = sqrt(x_axis_price). 
     We calculate a(t) and b(t) by taking the time-dependant linear interpolate between the initial and final values. 
     In other words, a(t) = (a_init * (1-t)) + (a_final * (t)) and b(t) = (b_init * (1-t)) + (b_final * (t)), where "t"
     is the percentage of time elapsed relative to the total specified duration. Since 
     a_init, a_final, b_init and b_final can be easily calculated from the input parameters (prices), this is a trivial
     calculation. config.a() and config.b() are then called whenever a and b are needed, and return the correct value for
     a or b and the time t. When the total duration is reached, t remains = 1 and the curve will remain in its final shape. 

Comment mismatch on min reserve ratio

It does not make good sense where any transaction that would result in the pool going above this ratio will fail, as far as the min reserve ratio is concerned. It should be corrected as follows:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L40

-           Min reserve ratio: The ratio between the two reserves cannot fall below a certain ratio. Any transaction that would result in the pool going above or below this ratio will fail.
+           Min reserve ratio: The ratio between the two reserves cannot fall below a certain ratio. Any transaction that would result in the pool going below this ratio will fail.

Comment mismatch on BASE_FEE

It is accurate by saying that the fee is applied twice.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L182-L186

    /** 
     @notice 
     Equivalent to roughly twenty-five basis points since fee is applied twice.
    */
    uint256 public constant BASE_FEE = 800;

This is because BASE_FEE is applied either in the if or the else block as is evidenced in the code logic below. Hence, the fee should be equivalent to roughly one half of twenty-five basis point.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L837-L847

        if (feeUp) {
            roundedAbsoluteAmount =
                absoluteValue +
                (absoluteValue / BASE_FEE) +
                FIXED_FEE;
            require(roundedAbsoluteAmount < INT_MAX);
        } else
            roundedAbsoluteAmount =
                absoluteValue -
                (absoluteValue / BASE_FEE) -
                FIXED_FEE;

Bit shifting should go beyond multiplication/division by two

It is known in the bot report that bit shifting is recommended for multiplication/division by two. However, this recommendation should go beyond all multiplication/division by a number comprising of n number of the factor two.

For instance, *4 in the code line below may be replaced by <<2:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L716

-        int256 disc = int256(Math.sqrt(uint256((bQuad**2 - (aQuad.muli(cQuad)*4)))));
+        int256 disc = int256(Math.sqrt(uint256((bQuad**2 - (aQuad.muli(cQuad)<<2)))));

Avoid operation when assigning values to constants

Consider assigning direct and discrete value to a constant instead of adopting mathematical/typecasting/etc operations. Otherwise the operation will have to be executed each time the constant is called. For example, instead using 10**9, use 1e9 or 1_000_000_000. Keep the operated codes in the comments though.

Here are the instances found.

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol

146:    uint256 constant INT_MAX = uint256(type(int256).max);

151:    int256 constant MIN_BALANCE = 10**12;

181:    uint256 constant MAX_BALANCE_AMOUNT_RATIO = 10**11;

191:    uint256 constant FIXED_FEE = 10**9;

201:    int256 constant MAX_PRICE_RATIO = 10**4; 

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.10",
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.

This report aims to provide a comprehensive analysis of the EvolvingProteus contract's codebase. Key issues, recommendations, and insights into the code's structure, quality, and potential risks are discussed. The analysis begins with a breakdown of specific findings, highlighting areas of concern, ranging from computational inconsistencies to potential financial discrepancies. The approach taken during this evaluation relied heavily on understanding the broader intentions of the protocol, aided by the provided documentation and links. While the codebase's overall structure was commendable, certain areas warranted attention, such as unreachable code blocks, coding style suggestions, and typographical errors. Furthermore, potential centralization and systemic risks were identified, emphasizing the need for continuous review and mitigation strategies. The report is complemented by a flowchart illustrating the contract's intricate relationships and dependencies. The overarching aim is to ensure the contract's robustness, transparency, and efficiency, safeguarding both developers and users alike.

Comments for the judge on my contextualized findings

I submitted a high and two medium findings, and herewith on their breakdown in the summary below:

1. Inconsistent Scaling in Quadratic Formula Computation (High): The _getUtility function, critical for calculating the utility of a pool based on a quadratic formula, contains a scaling inconsistency between its terms, potentially causing inaccuracies in the discriminant's computation. This can affect the contract's operation and mislead users, possibly resulting in financial discrepancies. Specifically, the scaling discrepancy between bQuad^2 (in 128.128 fixed-point format) and aQuad.muli(cQuad) * 4 (in 64.64 format) can make the discriminant falsely positive, avoiding expected complex solutions. To rectify this, it's recommended to refactor the terms for consistent scaling and update the inline documentation to clarify the expected scaling and any manual adjustments.
2. Final Amount to be Greater Than or Equal to the FIXED_FEE in _applyFeeByRounding Function NOT Guaranteed (Medium): The _applyFeeByRounding function is designed to impose a fee on transactions. However, a flaw in its fee calculation logic might result in a final amount that's less than the defined FIXED_FEE. Specifically, when the function deducts fees and the feeUp flag is false, the resultant amount, under certain conditions, can be smaller than FIXED_FEE. To address this issue, it's recommended to restructure the fee calculation or introduce added validations to ensure the final amount always meets or exceeds the FIXED_FEE. Proposed changes to the function are provided to rectify the discrepancy.
3. Lack of Deadline Protection in Key Functions Poses Potential Exploits (Medium): The EvolvingProteus contract lacks a crucial deadline parameter in key function calls, leading to a vulnerability where transactions might remain pending in the mempool due to outdated slippage and potentially get executed later than the user's intention. This delay can result in unfavorable trade outcomes and financial losses. The issue is compounded in PoS systems where block proposers are known in advance, providing an opportunity for malicious validators to exploit this oversight. Although the functions have slippage protections, they don't incorporate the essential deadline parameter. The report suggests refactoring the functions by adding a deadline parameter and verifying the current block timestamp against this deadline to prevent delayed transaction executions.

Approach taken in evaluating the codebase

As always, I started off by reading the docs and links provided to help gain a good understanding on the accounting and business intentions of the protocol. Covering both the in scope and the out of scope sections are necessary to help piece together all puzzles relating to the specific contract under audit in this contest. I agree with the general consensus that the codebase is very well structured, making it near to impossible to detect any critical vulnerabilities. Nonetheless, it is intriguing knowing that my high school math could be put to such a practical use in smart contract auditing that I have never imagined. Certainly there is a time for everything regardless of the contest results.

Architecture recommendations

I suggest having the contract under audit equipped with a complete set of NatSpec detailing all input and output parameters pertaining to each functions although much has been done in specifying each @dev. Additionally, a thorough set of flowcharts, and if possible a video walkthrough, could mean a lot to the developers/auditors/users in all areas of interests.

Codebase quality analysis

In analyzing the EvolvingProteus contract's codebase quality, several areas warrant attention: The function _getUtility possesses an unreachable 'if' block due to the properties of the square root function. There are coding style suggestions, including using uint256 over uint for clarity and using decimal instead of hexadecimal for readability. Several typographical errors exist in the code comments, and inconsistencies have been spotted in descriptions regarding BASE_FEE and reserve ratios. When the discriminant is zero, refactoring is recommended to calculate only one root. The contract is also susceptible to chain reorganization attacks on the Arbitrum L2 EVM-compatible chain. Bit shifting, a more efficient alternative to multiplication/division by factors of two, has been advised. Lastly, while slippage protection exists, deadline protection is suggested to complement it, preventing potential transaction delays that can affect trade outcomes, especially in PoS systems where block proposers are known in advance.

Centralization risks

There should not be much of a concern here considering the single contract under audit revolves around its dependency and trustworthiness in computational math. The onus lies on the calling contract on the key view functions. But I would suggest looking into some form of pausing/freezing visibilities (catering to emergencies) that have been deemed missing in both the in scope and the out of scope contracts.

Mechanism review

Here is a complete flow chart serving to augment a user's overall perceptions on how each function relates to one another both externally and internally. It also highlights the contract's connections to the imported libraries particularly on the key methods associated:

Systemic risks

Given that the contract has not gone live yet to be battle tested, systemic risks remain an unknown threat. I think continued efforts in getting additional mitigation reviews are of paramount importance to better safeguard the first of its kind deFi logic both in terms of atomicity and the gas reduced transactions. Perhaps, the proto-governance the protocol has already implemented through Toucan Votes could play a vital part in defraying the context of concern.

Time spent:

40 hours