Shell Protocol - Testerbot's results

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L577-L583

Vulnerability details

Impact

The contract has two constants MIN_M and MAX_M which act as the minimum and maximum slopes between the reserves of the pool, that is (balance of y reserve) / (balance of x reserve). Every time the reserves change, the new reserves should be validated to be within the limit specified by these constants.

The function _checkBalances is used for the purpose of making the validation:

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);
}

The issue is that the function _reserveTokenSpecified that is used in depositGivenInputAmount and in withdrawGivenOutputAmount does not call _checkBalances. The lack of validation will allow any user to deposit or withdraw an specified amount of tokens which may leave the ratio of the reserves out of the limits breaking an invariant of the protocol.

Proof of Concept

The other functions involved in swap and lp are _swap and _lpTokenSpecified both makes the validation to the reserve changes. This validation is not present in the code of _reserveTokenSpecified.

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the _reserveTokenSpecified as:

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;
      // @audit FIX
      _checkBalances(xi + specifiedAmount, yf);
   } else {
      yf = yi + _applyFeeByRounding(specifiedAmount, feeDirection);
      xf = xi;
      // @audit FIX
      _checkBalances(xf, yi + specifiedAmount);
   }
}

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);

Assessed type

Invalid Validation

L-01 There is no check that the discriminant is greater than zero.

The _getUtility function is used to compute the utility in the curve equation. To solve it for u, the function uses the quadratic formula. It starts by computing the discriminant:

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

It does not verify that bQuad**2 - (aQuad.muli(cQuad)*4) >= 0 before casting to uint256. Discriminants smaller than zero exist, meaning that the equation has its solutions in the complex numbers. This should not be the case here if the terms of the curve equation are correctly configured. Anyways, the discriminant should be validated to be greater than zero before casting it to uint256 to avoid any mistakes.

L-02 The contract only works for tokens with 18 Decimals.

Some assumptions around the code tell you that the code only works for tokens with 18 decimals. For example, the following constant:

uint256 constant FIXED_FEE = 10**9;

This constant is a flat fee charged for every swap and liquidity addition or substraction. But, if a token were USDC, this fee means 1000 USDC, which is unreasonable.

NC-01 The function _getUtility makes an innecessary check:

The following line can be removed:

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

a and b are never going to be smaller than zero.

NC-01 Incorrect comment in swapGivenInputAmount

// amount cannot be less than 0
require(result < 0);

The correct comment should be amount cannot be greater than 0

NC-02 Incorrect comment in withdrawGivenInputAmount

// * @dev We use FEE_UP because we want to increase the perceived amount of
// *  reserve tokens leaving the pool and to increase the observed amount of
// *  LP tokens being burned.

Should be change to something similar as:

// We use FEE_DOWN because we want to decrease the perceived amount of 
// LP tokens to burn and to decrease the observed reserve tokens taken out