Shell Protocol - Udsen's results

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L563-L595 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L809-L814 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L549 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L552 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L641 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L644

Vulnerability details

Impact

The EvolvingProteus.depositGivenInputAmount function is used to calculate the output amount of LP tokens given an input amount of reserve tokens.

The EvolvingProteus.withdrawGivenOutputAmount function is used to calculate the amount of LP tokens that must be burned given an output amount of reserve token.

Both above functions will calculate the changed LP token supply proportionately to the change in the utility of the pool.

In the function execution of the above two functions (depositGivenInputAmount and withdrawGivenOutputAmount) the _reserveTokenSpecified internal function is called.

The _reserveTokenSpecified function is used to compute the changed amount of the LP token supply based on the proportional change of the utility of the pool.

The issue here is that after the LP token total supply change calculation, there is no check performed on the token balance reserves and balance reserve ratio. Hence this function does not verify whether the reserve balances are within the valid boundaries once the transaction is performed.

Both the EvolvingProteus._swap function and the EvolvingProteus._lpTokenSpecified functions ensure that reserve balances are within valid boundaries once the respective transactions are completed by calling the EvolvingProteus._checkBalances private function. But the _reserveTokenSpecified function does not call on the EvolvingProteus._checkBalances function to verify whether the reserve balances are within the valid boundaries once the transaction is completed.

The EvolvingProteus.depositGivenInputAmount transaction will deposit an amount of specified token amount, which could decrease the y.divi(x) (here y is the non-specified token amount and x is the specified token amount deposited) below the MIN_M value (since x is increasing as its getting deposited). Hence the transaction should revert since the reserve token balance ratio is invalid. But the transaction will execute successfully because there is no check performed to identify this issue.

Similarly when calling the EvolvingProteus.withdrawGivenOutputAmount transaction, an amount of specified token will be withdrawn from the pool. This will decrease the specified token balance of the pool and could increase the y.divi(x) ratio (since x is the specified token and it is getting decreased thus increasing the ratio) beyond the MAX_M value. Hence the transaction should revert since the reserve token balance ratio is invalid. But the transaction will execute successfully because there is no check performed to identify this issue.

Hence it is clear lack of reserve balance checks in the _reserveTokenSpecified function breaks the key invariants related to the reserve token balances and reserve token balance ratio during deposit and withdraw transactions. Hence this breaks key behaviour of the protocol.

Proof of Concept

    function _reserveTokenSpecified(
        SpecifiedToken specifiedToken,
        int256 specifiedAmount,
        bool feeDirection,
        int256 si,
        int256 xi,
        int256 yi
    ) internal view returns (int256 computedAmount) {
        int256 xf;
        int256 yf;
        int256 ui;
        int256 uf;
        {
            // calculating the final price points considering the fee
            if (specifiedToken == SpecifiedToken.X) {
                xf = xi + _applyFeeByRounding(specifiedAmount, feeDirection);
                yf = yi;
            } else {
                yf = yi + _applyFeeByRounding(specifiedAmount, feeDirection);
                xf = xi;
            }
        }

        ui = _getUtility(xi, yi);
        uf = _getUtility(xf, yf);

        uint256 result = Math.mulDiv(uint256(uf), uint256(si), uint256(ui));
        require(result < INT_MAX);   
        int256 sf = int256(result);

        // apply fee to the computed amount
        computedAmount = _applyFeeByRounding(sf - si, feeDirection);
    }

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

    function _checkBalances(int256 x, int256 y) private pure {
        if (x < MIN_BALANCE || y < MIN_BALANCE) revert BalanceError(x,y);
        int128 finalBalanceRatio = y.divi(x);
        if (finalBalanceRatio < MIN_M) revert BoundaryError(x,y);
        else if (MAX_M <= finalBalanceRatio) revert BoundaryError(x,y);
    }

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

            _checkBalances(xi + specifiedAmount, yi + computedAmount);

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

            _checkBalances(xi + computedAmount, yi + specifiedAmount);

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

            computedAmount = _applyFeeByRounding(xf - xi, feeDirection);

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

            computedAmount = _applyFeeByRounding(yf - yi, feeDirection);

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

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

It is recommended to call the EvolvingProteus._checkBalances function with the relevant parameters at the end of the _reserveTokenSpecified function, to verify once the deposit or withdraw transaction is over the reserve balances and reserve balance ratio are within the valid boundaries of the protocol.

Assessed type

Other

1. RECOMMENDED TO ADD AN INPUT VALIDATION FOR THE duration VARIABLE IN THE constructor

In the EvolvingProteus.constructor the duration is passed in as an input parameter. duration denotes the total duration of the curve's evolution. Hence the duration variable is a critical variable of the protocol. If duration is set to zero by mistake it will prompt a redeployment of the contract which could be wastage of gas and additional work load. Hence an input validity check should be performed for the duration variable inside the constructor as follows:

require(duration != 0, `Duration can not be zero`);

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

2. DISCREPENCY BETWEEN NATSPEC COMMENTS AND LOGIC IMPLEMENTATION

The EvolvingProteus._getUtility function is used to calculate the utility value of the pool at the time of the function call. The utilitiy is calculated using a quadratic formula which is shown below:

k(ab - 1)u**2 + (ay + bx)u + xy/k = 0

As per the equation there is a k value used in the a adn c coefficients. But this k value is not used when calculating the aQuad and the cQuad values. This could be due to normalization of the equation or the k = 1. But the reason for the missing k value from the coefficient calculations is not given.

Hence it is fair to assume there is a discrepency between the Natspec comments and the actual implementation of the logic.

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

3. DISCREPENCY BETWEEN VALUES uint256 CAN HOLD IN EvolvingProteus CONTRACT DUE TO INT_MAX CAP

In the EvolvingProteus contract the input parameter amounts to the transaction functions such as swap, deposit and withdraw are capped by INT_MAX value. The input parameters are required to be less than this amount as shown below:

    require(
        burnedAmount < INT_MAX &&
            xBalance < INT_MAX &&
            yBalance < INT_MAX &&
            totalSupply < INT_MAX
    );

Hence the any of the passed in input parameters can not be equal to the INT_MAX value.

But when deriving an output amount using helper functions of this contract, the resultant amounts upcasted from int256 to uint256 value. When it is converted into a uint256 value those parameters are allowed to have a value = INT_MAX. This is because upcasting does not lead to data truncation. But it does not treat all the uint256 variables in the function scope equally since the uint256 values calculated inside the function scope can have the type(int256).max value where as the passed in uint256 input parameters can not be equal to the type(int256).max value.

Hence there is a discrepency between the amounts that an uint256 can hold inside this smart contract. This discrepency in uint256 allowed values can lead to precision loss and other unexpected behaviours of the contract.

Hence it is recommended to either allow the passed in uint256 input parameters to the functions, to have a value equal to type(int256).max, or prohibit internal uint256 variables of the EvolvingProteus contract from having values equal to type(int256).max value.

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

4. DISCREPENCY BETWEEN NATSPEC COMMENT AND LOGIC IMPLEMENTATION OF THE withdrawGivenInputAmount FUNCTION

The natspec comments of the EvolvingProteus.withdrawGivenInputAmount function states that the feeDirection of the function to be used is FEE_UP. The reason for using FEE_UP is described as wanting to increase the perceived amount of reserve tokens leaving the pool. But the feeDirection is determined by the protocol to benefit itself. Hence withdrawGivenInputAmount transaction will benefit the protocol if perceived amount of reserve tokens leaving the pool is decreased which is equivalent to charging a fee from the withdrawing amount.

The logic implemenation of the withdrawGivenInputAmount function has implemented this requirement correctly as shown below:

    int256 result = _lpTokenSpecified( //@audit-info - amount of reserved token to withdraw
        withdrawnToken,
        -int256(burnedAmount),
        FEE_DOWN, //@audit-issue - discrepency between the NATSPEC and the implementation regarding FEE_DOWN
        int256(totalSupply),
        int256(xBalance),
        int256(yBalance)
    );

Hence it is recommended to update the natspect comments of the withdrawGivenInputAmount function to state that the feeDirection used for the withdrawGivenInputAmount transaction is FEE_DOWN by providing it with the proper reason to do so as well.

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

5. ERRORNEOUS NATSPEC COMMENTS SHOULD BE CORRECTED

@param px_final The final price at the `y axis`

Above comment should be corrected as follows:

@param px_final The final price at the `x axis`

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

The minimum price value calculated with abdk library equivalent to `10^12(wei)`

The above comment should be corrected as follows since the given value (10^12) in the comment is not the minimum price value

The minimum price value calculated with abdk library equivalent to `10^10(wei)`

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

 *  without caring about which particular function `is was` called.

The above comment should be corrected as follows:

 *  without caring about which particular function `was` called.

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

6. LACK OF EVENTS IN THE CRITICAL OPERATIONS OF THE PROTOCOL

The EvolvingProteus smart contract performs critical operations such as swaps, deposits, withdrawals. But these operations lack events. Hence it is recommended to emit events when above operations are succefully executed for off-chain analysis.

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

7. ADD MEANINGFUL revert MESSAGES TO THE require STATEMENTS

In the EvolvingProteus contract, there are multiple occassions require statement are used for conditional checks. But these require statements are not using revert messages. Hence it these require statements revert it will be difficult to find the reason for the revert.

Hence it is recommneded to add meaningful revert messages to these require statements.

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

Summary

The EvolvingProteus is a solution to passively update the liqudity concentration over time. It can be thought of as a hybrid implementation between Balancer liquidity bootstrapping pool and Uniswap v3. Here the starting liquidty concentration and ending liquidty concentration is set by the creator who sets the starting timestamp and ending timestamp as well. Within this duration the liquidity concentratino evolves.

The main smart contract which is within the scope for this audit is EvolvingProteus.sol.

The EvolvingProteus.sol is a smart contract that implements a liquidity pool with a time-dependent bonding curve. The bonding curve is used to determine the price of tokens in the pool based on the current supply. The curve evolves over time according to the parameters specified at the contract's deployment.

The contract uses the ABDKMath64x64 library for fixed-point arithmetic and the OpenZeppelin Math library for basic mathematical operations. It also imports an interface for a liquidity pool implementation and a specified token type.

The contract includes functions for swapping tokens, depositing tokens into the pool, and withdrawing tokens from the pool. Each of these functions can be called with either a specified input amount or a specified output amount. The contract calculates the corresponding output or input amount, respectively, based on the current state of the bonding curve.

The contract also includes a library, LibConfig, which is used to calculate the parameters of the bonding curve at any given time.

The contract includes several checks to ensure that transactions do not result in an invalid state. For example, it checks that the amount of tokens being swapped, deposited, or withdrawn is not too small or too large relative to the size of the reserves in the pool. It also checks that the ratio between the two reserves does not fall below or exceed certain limits.

Any comments for the judge to contextualize your findings

The majority of my findings have been based on how the key invariants are broken due to edge cases that EvolvingProteus.sol contract could face. A brief summary of each of the findings is given below:

H1 : How the absoluteValue < FIXED_FEE * 2 key invariant is broken with in the _applyFeeByRounding function. This function is used to charge fees by rounding values.

H2 : How the key invariants related to reserve balances in the _checkBalances function can be broken due to the usage of errorneous values.

H3: _reserveTokenSpecified function does not check whether reserve balance invariants hold after the function exeuction is completed.

M1: The utility is calculated using a quadratic equation. It's solution gives two answers based on the sign of the discriminant. The unsafe casting of discriminant to uint256 value could result in a errorneous value thus providing wrong output

M2: Condtional check which excludes the exact value finalBalanceRatio in the _checkBalances function is errorneous

M3 : Important conditinal check to ensure final LP token supply is greater than the MIN_BALANCE is not performed inside the _reserveTokenSpecified. Hence the respective invariant could be broken.

Approach taken in evaluating the codebase

I initially started understanding the protocol by referring the documentation. I initially learnt the key workings of the EvolvingProteus solution. Then I understood the key applications of the EvolvingProteus solution such as the Dutch auctions and Dynamic pools.

Then I learnt about the key parameters of the solution related to prices, liquidity concentration and timestamps. Then I learnt how these parameters are interconnected through the time-weighted interpolation of prices, bonding curve equation related to the liquidity concentration and quadratice equation related to the utility calculations.

Then I analyzed the key invariants of the protocol, price range limitations and types of errors anticipated in the protocol.

After gaining enough knowledge about the protocol I started auditing the EvolvingProteus.sol smartcontract (Inscope smartcontract). I initially focussed on the state changing functions. I analyzed whether it is possible to change the state of the contract to an invalid state via those functions.

Then I focused on key invariants of the contract. I tries to think of various methods and edge cases in breaking the given invariants. I was able to find several vulnerabilities related to breaking invariants in edge cases.

Later I checked whether all the required conditional checks and invariant checks are performed to ensure the contract states are within required boundaries after specific transactions are sucesfully executed.

By following the above mentioned approach I was able to find vulnerabilities of this project.

Architecture recommendations

The architecture used in the EvolvingProteus solution is a robust one. It is heavily dependent on mathematics. It uses a novel mehtod of evolving liquidity concentration across a predetermined time duration. A bonding curve equation is used to calculate the current liquidity concentration by taking into consideration the price of tokens (in the units of numeraire token) obtained via time-weighted interpolation of prices. The utility is used as the measurement of value of the liquidity pool. The utility is calculated based on a quadratic equation.

Codebase quality analysis

Considering the complexity of the algorithms associated with the protocol and interlinks among different components of the procotol such as pricing, liquidity concentration, utility, it must be stated that the codebase quality is high and commendable.

It is recommended to focus more on invariant checks and verify that the key invariants are held after each swap, deposit or withdraw transaction is completed. It is also recommended to implement logic in the EvolvingProteus.sol contract to ensrue key invariants are not broken via edge cases, becuase few vulnerabilities were found related to this during the audit.

Since there are many hard and soft invariants in this protocol it is recommended to focus more on foundry invariant tests to ensure the codebase is highly robust.

More attention can be paid on the Natspec comments since there were numerous typos found in them which I have included in my QA report. Since developers and auditors rely on the Natspec comments to get a general idea about the codebase, accurate Natspec commnets will increase the quality of the codebase.

Centralization risks

There were no centralization risks found within the EvolvingProteus.sol smart contract. The external functions of the contract were used for swaps, deposits and withdrawals. They were not controlled via any access control. And these external functions were view functions used for calculations.

Further the internal functions used as helper functions for above external functions were also view functions and were not controlled by any access control.

Hence there was no risk of centralization since there were no admin controlled functions or any role based access control within these functions of the EvolvingProteus.sol smart contract.

Mechanism review

The EvolvingProteus.sol contract and the EvolvingProteus solution is heavily dependent on mathematics. There are bonding curves, time-weighted interpolation and quadratic equations used for various calculations within its scope. The majority of calculations were based on fixed-point arithmetic using the ABDKMath64x64 library. Hence extra attention needed to be paid on the decimal precision and accuracy of resultant values of the calculations.

Since the EvolvingProteus solution is similar to a hybrid solution of liquidity bootstrapping pools and Uniswap V3, the solution can be used for price discovery of newly created tokens and to execute large buy or sell order without significant price impact. The passive liqudity concentration management of the solution can be used to allow the LPs to earn more fees on thier provided liquidity since thier liquidity will be concentrated near the spot price of the asset continously.

As a result the uses cases of this protocol are on the Dutch Auctions and Dynamic Pools, which can make use of the above mentioned features of the protocol.

Systemic risks

The systemic risks of this protocol are related to breaking of key invariants under edge cases. Possible errors related to calculations of fixed-point arithmetic and its precision and accuracy.

Missing checks to verify key invariants are holding after succesful execution of the deposit, withdraw and swap operations could lead to vulnerabilities.

This contract uses Fixed-point arithmatic. Hence the precision errors, rounding errors, underflow and overflow errors could occur. Hence need to pay extra attention on above areas when dealing with arithmatic operations in this contract

In Fixed-point arithmatic if you dedicate more bits to the fractional part for higher precision, you reduce the number of bits available for the integer part, limiting the range of whole numbers you can represent. Conversely, if you want to represent a larger range of whole numbers, you reduce the precision of the fractional part.

Time spent:

25 hours