Shell Protocol - markus_ether's results

Lines of code

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

Vulnerability details

Impact

When depositing or withdrawing liquidity and specifying the amount of reserve tokens, the protocol bypasses crucial checks ensuring the amounts locked don't become too small. This oversight allows a malicious actor to remove all but a negligible quantity of liquidity from the pool, rendering the trading pool ineffectual. With low and imbalanced liquidity, rounding errors escalate, causing losses for liquidity providers.

Proof of Concept

When depositing or withdrawing liquidity by specifying the LP tokens, the flow enforces the function _checkBalances at the end of the execution.

The code enforces the following conditions:

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

In simpler terms: The balances of x and y must exceed a specific minimum threshold, and the ratio of y/x should fall within a defined range.

This check is overlooked when specifying the amount of reserve tokens. Amounts of reserve tokens are specified when calling depositGivenInputAmount or withdrawGivenOutputAmount. Thus, it's possible to shift the pool into an insecure state that breaches the designated safety zones. The proof of concept below demonstrates a scenario where a user withdraws all but 1 Wei of token X from the pool:

function testWithdrawAboveLimit(uint256 x0, uint256 y0, uint256 s0) public {
    vm.assume(x0 >= uint256(MIN_BALANCE));
    vm.assume(y0 >= uint256(MIN_BALANCE));
    vm.assume(s0 >= uint256(MIN_BALANCE));
    vm.assume(y0/x0 <= MAX_BALANCE_AMOUNT_RATIO);
    vm.assume(x0/y0 <= MAX_BALANCE_AMOUNT_RATIO);

    // deploy with default
    proteus = new EvolvingInstrumentedProteus(
    ABDKMath64x64.divu(695000000000000, 1e18),
    ABDKMath64x64.divu(692930500000000, 1e18),
    ABDKMath64x64.divu(695000000000000, 1e18),
    ABDKMath64x64.divu(692930500000000, 1e18),
    3 days
);

    // attempt to withdraw all (but 1 wei) of the provided x liquidity
    SpecifiedToken token = SpecifiedToken.X;
    uint256 witdrawnAmount = x0 - 1;

    try proteus.withdrawGivenOutputAmount(x0, y0, s0, witdrawnAmount, token) returns (uint256 tokenBurned) {
        if (tokenBurned < s0 && witdrawnAmount >= MIN_OPERATING_AMOUNT) {
            uint256 x1 = x0 - witdrawnAmount;
            uint256 y1 = y0;
            assertGe(x1, uint256(MIN_BALANCE), "min balance");
            assertLe(y1 / x1, MAX_BALANCE_AMOUNT_RATIO, "wrong ratio, x too small");
            assertLe(x1 / y1, MAX_BALANCE_AMOUNT_RATIO, "wrong ratio, x too large");
        }
    } catch {}
}

Once liquidity is removed, further LPs are prevented from depositing or withdrawing when specifying the LP tokens via withdrawGivenInputAmount or depositGivenOutputAmount since these functions impose the check. Additionally, the rounding errors in the protocol surpass the anticipated values for current Liquidity Providers and Traders, leading to direct losses for Liquidity Providers.

Tools Used

Manual review

Recommended Mitigation Steps

Incorporate the condition at the end of the function:

function _lpTokenSpecified(
...
) internal view returns (int256 computedAmount) {
+    // balance checks with consideration the computed amount
+    if (specifiedToken == SpecifiedToken.X) {
+        computedAmount = _applyFeeByRounding(xf - xi, feeDirection);
+        _checkBalances(xi + computedAmount, yf);
+    } else {
+        computedAmount = _applyFeeByRounding(yf - yi, feeDirection);
+        _checkBalances(xf, yi + computedAmount);
+    }
}

Assessed type

Invalid Validation