Unfair arbitrage for users, allowing protocol to gain more than should

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L62 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L72

Vulnerability details

I have foreseen this report to be controversial. But hear me out from the user's perspective. Salty protocol uses AAA for market inefficiencies that are arbitraged at swap time to create profits. Now the objective is to ensure users have fair tokens return for being liquidity providers. Everyone should want to receive as many output tokens as possible for an exact input amount.

In the case that tokensA or tokensB deposited are not in proportion, it will be adjusted. The problem lies in the adjustment of tokensA and tokenB proportion deposited.

User calls Liquidity.soL::depositLiquidityAndIncreaseShare() which will subsequently call the internal function of Liquidity.sol::_dualZapInLiquidity().

Within that internal function the Pool.sol::depositSwapWithdraw() is called with the minAmountOut set as 0. To allow the protocol to perform arbitrage. This shouldn't be set to 0, instead, it should get the minimum amount swapped for the users to ensure that arbitrage is completely fair.

Impact

Users will get lesser tokenA or tokenB when internally swapped, hence loss of funds.

Proof of Concept

    function _dualZapInLiquidity(IERC20 tokenA, IERC20 tokenB, uint256 zapAmountA, uint256 zapAmountB ) internal returns (uint256 amountForLiquidityA, uint256 amountForLiquidityB  )
        {
        (uint256 reserveA, uint256 reserveB) = pools.getPoolReserves(tokenA, tokenB);
        (uint256 swapAmountA, uint256 swapAmountB ) = PoolMath._determineZapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB );

        // tokenA is in excess so swap some of it to tokenB?
        if ( swapAmountA > 0)
            {
            tokenA.approve( address(pools), swapAmountA );

            // Swap from tokenA to tokenB and adjust the zapAmounts
            zapAmountA -= swapAmountA;
            zapAmountB += pools.depositSwapWithdraw( tokenA, tokenB, swapAmountA, 0, block.timestamp ); //@audit

        // tokenB is in excess so swap some of it to tokenA?
        else if ( swapAmountB > 0)
            {
            tokenB.approve( address(pools), swapAmountB );

            // Swap from tokenB to tokenA and adjust the zapAmounts
            zapAmountB -= swapAmountB;
            zapAmountA += pools.depositSwapWithdraw( tokenB, tokenA, swapAmountB, 0, block.timestamp );//@audit
            }

        return (zapAmountA, zapAmountB);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Create a quote function to get the latest minimum price. 1 external for gas optimization and 1 internal to be used for the _dualZapInLiquidity. Then set the minAmountOut price within the Pools.sol::depositSwapWithdraw parameter. Mitigation

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #805

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid