code-423n4 / 2023-08-shell-findings

3 stars 2 forks source link

`withdrawGivenInputAmount()` calls `_lpTokenSpecified` with the wrong value of `feeDirection` #217

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

withdrawGivenInputAmount() calls _lpTokenSpecified with the wrong value of feeDirection:

function withdrawGivenInputAmount(
        uint256 xBalance,
        uint256 yBalance,
        uint256 totalSupply,
        uint256 burnedAmount,
        SpecifiedToken withdrawnToken
    ) external view returns (uint256 withdrawnAmount) {
        // lp amount validations against the current balance
        require(
            burnedAmount < INT_MAX &&
                xBalance < INT_MAX &&
                yBalance < INT_MAX &&
                totalSupply < INT_MAX
        );

        int256 result = _lpTokenSpecified(
            withdrawnToken,
            -int256(burnedAmount),
            FEE_DOWN,   
            int256(totalSupply),
            int256(xBalance),
            int256(yBalance)
        );

        // amount cannot be less than 0
        require(result < 0);
        withdrawnAmount = uint256(-result);
    }

This will affect the fee application logic and consequently, the final computed amount for withdrawals.

If FEE_DOWN is used in the withdrawal context instead of FEE_UP, it would apply the fee in the opposite direction than intended. This might give users slightly more tokens upon withdrawal than they should have received or reduce the fee impact.

Proof of Concept

The purpose of FEE_UP and FEE_DOWN is to be to adjust the fee direction. From the NatSpec above function, we see:

     * @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.

If the fee is reduced or not applied as intended, the pool might give out more reserve tokens than it should during a withdrawal. This would impact the reserve balance and possibly the pool's overall "utility" or invariant. Over time and with many withdrawals, this might pose a liquidity risk or a potential imbalance in the pool's reserves.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

In withdrawGivenInputAmount() function when calling _lpTokenSpecified() need to change FEE_DOWN with FEE_UP.

Assessed type

Other

c4-pre-sort commented 1 year ago

0xRobocop marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xRobocop marked the issue as low quality report

0xRobocop commented 1 year ago

Invalid.

Code is correct, comments are do not.

c4-judge commented 1 year ago

JustDravee marked the issue as unsatisfactory: Invalid