Malicious user can drain the reserve during swaping and withdrawal

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L548 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L551 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L506-L554 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L288-L303 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L299-L303 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L452

Vulnerability details

Impact

A malicious user can drain pool reserves by calling the swapGivenInputAmount(...) function during swapping and also during withdrawal by calling withdrawGivenOutputAmount(...) and withdrawGivenInputAmount(...) during withdrawal leading to a loss of funds.

Proof of Concept

during swapping, the swapGivenInputAmount(...) function makes a call to _swap(...) which in turn makes a call to _applyFeeByRounding(...)

    function _swap(
        bool feeDirection,
        int256 specifiedAmount,
        int256 xi,
        int256 yi,
        SpecifiedToken specifiedToken
    ) internal view returns (int256 computedAmount) {
        ...

        if (specifiedToken == SpecifiedToken.X) {
            computedAmount = _applyFeeByRounding(yf - yi, feeDirection);
            ...
        } else {
            computedAmount = _applyFeeByRounding(xf - xi, feeDirection);
            ...
        }
    }

although for swaps, if SpecifiedToken is X, then yf - yi is negative (vice versa) and _applyFeeByRounding(...) handles that correctly and returns a positive swap output amount that is cached in result which is a signed integer

    function _applyFeeByRounding(int256 amount, bool feeUp)
        private
        pure
        returns (int256 roundedAmount)
    {
        bool negative = amount < 0 ? true : false;
        ...

        roundedAmount = negative
            ? -int256(roundedAbsoluteAmount)
            : int256(roundedAbsoluteAmount);
    }

However, the swapGivenInputAmount(...) checks that the result is less than zero before proceeding to calculate the outputAmount which also is evaluated by first casting and then negating the value of result. this can lead an amount lower than the actual outputAmount.

The same vulnerability described above is present in the withdrawGivenOutputAmount(...) function as shown below

    function withdrawGivenOutputAmount(
        uint256 xBalance,
        uint256 yBalance,
        uint256 totalSupply,
        uint256 withdrawnAmount,
        SpecifiedToken withdrawnToken
    ) external view returns (uint256 burnedAmount) {
        ...;

        int256 result = _reserveTokenSpecified(
            withdrawnToken,
            -int256(withdrawnAmount),
            FEE_UP,
            int256(totalSupply),
            int256(xBalance),
            int256(yBalance)
        );

        ...
        burnedAmount = uint256(-result);
    }

Exploit Scenario

Lets assume when swapping a SpecifiedToken.X units amount, the calculated yToken output is 7units.

When negating signed integer values before casting to unsigned integers, values can increase or decrease astronomically, let assume _swap(...) returns the signed integer +7 cached in result as shown below,

    function swapGivenInputAmount(
        uint256 xBalance,
        uint256 yBalance,
        uint256 inputAmount,
        SpecifiedToken inputToken
    ) external view returns (uint256 outputAmount) {
        ...

        int256 result = _swap(
            FEE_DOWN,
            int256(inputAmount),
            int256(xBalance),
            int256(yBalance),
            inputToken
        );
        ...

        // output amount validations against the current balance
        outputAmount = uint256(-result);
        ...
    }

when casting result to an unsigned integer, firstly the logic negates this signed integer value to -7 before casting and in doing this, the integer is converted to a negative signed value i.e for example from the value

000...00111

to the value

100...00111

and then the value uint256(-7) is calculated as

(2**555 + 2**2 + 2**1 + 2**0)

the result has now been astronomically increased from 7unit of output token amount to a very large value (~5e58) this shows that even a small swap amount can lead to having a astronomical output amount a malicious user only needs two transactions to drain both reserves by swapping X for Y and then Y for X.

Tools Used

Manual Review

Recommended Mitigation Steps

In the swapGivenInputAmount(...) function

• remove the minus sign
• change the less than sign in the require statement to greater than
• add a check to ensure outputAmount is not more than the maximum swap amount. as shown below

    function swapGivenInputAmount(
        uint256 xBalance,
        uint256 yBalance,
        uint256 inputAmount,
        SpecifiedToken inputToken
    ) external view returns (uint256 outputAmount) {

        require(result > 0);

        // output amount validations against the current balance
        outputAmount = uint256(result);
        ...
    }

In the withdrawGivenOutputAmount(...) function

• remove the minus sign.

• change the less than sign in the require statement to greater than. as shown below.

  function withdrawGivenOutputAmount(
      uint256 xBalance,
      uint256 yBalance,
      uint256 totalSupply,
      uint256 withdrawnAmount,
      SpecifiedToken withdrawnToken
  ) external view returns (uint256 burnedAmount) {
      ...;
  
      int256 result = _reserveTokenSpecified(
          withdrawnToken,
          -int256(withdrawnAmount),
          FEE_UP,
          int256(totalSupply),
          int256(xBalance),
          int256(yBalance)
      );
  
      require(result > 0);
      burnedAmount = uint256(-result);
  }

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[0xRobocop]

Insufficient proof

Incorrect premises:

Lets assume when swapping a SpecifiedToken.X units amount, the calculated yToken output is 7units. When negating signed integer values before casting to unsigned integers, values can increase or decrease astronomically, let assume _swap(...) returns the signed integer +7 cached in result as shown below,

In the context of swapGivenInputAmount the value returned by _swap cannot be positive, this is enforced via:

require(result < 0);

[c4-judge]

JustDravee marked the issue as unsatisfactory: Insufficient proof