Shell Protocol - ItsNio's results

Lines of code

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

Vulnerability details

Impact

By calling depositGivenInputAmount() and withdrawGivenOutputAmount() (which both call '_reserveTokenSpecified()'), users may potentially create scenarios where the balance ratio allowed for the EvolvedProtocol.sol deployment is violated.

POC

depositGivenInputAmount() and withdrawGivenOutputAmount() return the amount of LP tokens minted/burned given a deposit/withdrawal of some reserve tokens. They both rely on the internal function _reserveTokenSpecified() for this calculation. In contrast, the other deposit and withdrawal functions rely on the internal function_lpTokenSatisfied().

Within _lpTokenSatisfied(), there is a check _checkBalances() after the deposit/withdraw that ensures that the supplies of token x and token y reserve tokens are above minimum amounts and also satisfy a balance ratio.

This check is missing completely within the main bodies of depositGivenInputAmount() and withdrawGivenOutputAmount() as well as _reserveTokenSpecified(), hence it is possible to create a scenario where the final result after a deposit/withdrawal does not satisfy the balance ratio, yet the deposit/withdrawal does not revert.

We have created a runnable test to demonstrate the POC, however, it requires some minor changes to the code body, detailed below:

Preconditions

To run the POC, the following things should be done:

1. Within EvolvingProteus.sol, add the following two functions. These two functions are a nearly identical copy of the withdrawGivenOutputAmount() and _reserveTokenSatisifed() function, with the only difference being a toggleable _checkBalances(xi + specifiedAmount, yf) function call to demonstrate the impact of the bug. The reasoning for the parameters of the function call being xi + specifiedAmount instead of xf is to keep the withdraw consistant with the _swap() function in favoring the pool during fee rounding.

function withdrawGivenOutputAmountWithChecks(
        uint256 xBalance,
        uint256 yBalance,
        uint256 totalSupply,
        uint256 withdrawnAmount,
        SpecifiedToken withdrawnToken,
        bool checksOn //@audit toggleable bool
    ) external view returns (uint256 burnedAmount) {
        // withdraw amount validations against the current balance
        require(
            withdrawnAmount < INT_MAX &&
                xBalance < INT_MAX &&
                yBalance < INT_MAX &&
                totalSupply < INT_MAX
        );
        int256 result = _reserveTokenSpecifiedWithChecks(
            withdrawnToken,
            -int256(withdrawnAmount),
            FEE_UP,
            int256(totalSupply),
            int256(xBalance),
            int256(yBalance),
            checksOn //@audit toggleable bool
        );
        // amount cannot be less than 0
        require(result < 0);
        burnedAmount = uint256(-result);
    }
function _reserveTokenSpecifiedWithChecks(
        SpecifiedToken specifiedToken,
        int256 specifiedAmount,
        bool feeDirection,
        int256 si,
        int256 xi,
        int256 yi,
        bool checksOn
    ) 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;
            }
        }

        //@audit: This is the only difference between the other `_reserveTokenSpecified
        if(checksOn){
                _checkBalances(xi + specifiedAmount, yf);
        }

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

2. Within EvolvingProteus.t.sol in the test directory, add the following 2 tests anywhere:

function testImproperWithdrawGO(
        uint256 x0,
        uint256 y0,
        uint256 s0,
        uint256 depositedAmount,
        bool toke
    ) public {
        //One test case (to demonstrate the result)
        x0 = 10**22;
        uint256 _y0 = 10**12;
        s0 = MIN_BALANCE;//10**10;
        y0 = _y0;
        emit log("RUNNING!");
        vm.assume(x0 >= MIN_BALANCE);
        vm.assume(y0 >= MIN_BALANCE);
        vm.assume(s0 >= MIN_BALANCE);
        vm.assume(y0/x0 <= MAX_BALANCE_AMOUNT_RATIO);
        vm.assume(x0/y0 <= MAX_BALANCE_AMOUNT_RATIO);
        uint256 withdrawnAmount = x0*6/10;
        SpecifiedToken withdrawnToken = SpecifiedToken.X;
        //emit log_int(int256(x0));
        DUT.withdrawGivenOutputAmount(
                x0,
                y0,
                s0,
                withdrawnAmount,
                withdrawnToken
            );
    }
function testProperWithdrawGO(
        uint256 x0,
        uint256 y0,
        uint256 s0,
        uint256 depositedAmount,
        bool toke
    ) public {
        //One test case (to demonstrate the result)
        x0 = 10**22;
        uint256 _y0 = 10**12;
        y0 = _y0;
        s0 = MIN_BALANCE;

        bool checksOn = true;  //@audit Toggle this to turn checks on (true) or off (false)
        //@audit: When the checks are on, the test fails due to `_checkBalances` failing.
        //@audit: When the checks are off, the tests pass normally, exactly the same as the test `testImproperWithdrawGO`

        vm.assume(x0 >= MIN_BALANCE);
        vm.assume(y0 >= MIN_BALANCE);
        vm.assume(s0 >= MIN_BALANCE);
        vm.assume(y0/x0 <= MAX_BALANCE_AMOUNT_RATIO);
        vm.assume(x0/y0 <= MAX_BALANCE_AMOUNT_RATIO);
        uint256 withdrawnAmount = x0*6/10;
        SpecifiedToken withdrawnToken = SpecifiedToken.X;
        DUT.withdrawGivenOutputAmountWithChecks(
                x0,
                y0,
                s0,
                withdrawnAmount,
                withdrawnToken,
                checksOn
            );

        //vm.expectRevert();
    }

To run the tests, either run forge test or forge test --match-contract EvolvingProteusProperties --match-test testImproperWithdrawGO forge test --match-contract EvolvingProteusProperties --match-test testProperWithdrawGO to specifically run each test

Expected output and explanation

testImproperWithdrawGO() is a test that runs on the unchanged versions of withdrawGivenOutputAmount() and _reserveTokenSpecified(), while testProperWithdrawGO() uses the newly inserted versions of the two functions.

testImproperWithdrawGO() and testProperWithdrawGO() when checksOn is toggled to false both run without reporting any issues. Both of these cases represent when the current withdrawGivenOutputAmount() is called with a scenario that violates the price range.

When checksOn is toggled to true, testProperWithdrawGO() will fail, as the _checkBalances() function is able to detect the price range violation.

Recommended Mitigation

Add the _checkBalances() function call within the _reserveTokenSpecified() function. One possible solution could be to directly use the provided code above.

Assessed type

Math