Following a single Fix arbitrage path for particular swaps can lead to lesser arbitrage profits,faliure to detect arb opportunities and pools being imbalanced for longer periods

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/arbitrage/ArbitrageSearch.sol#L101-L136

Vulnerability details

Impact

Lesser Arbitrage profits and faliure to detect arb opportunities

Proof of Concept

The following pools are existing in correct ratio- 1 wbtc : 40000 dai (1 wbtc = 40000 \$) 1 wbtc : 16 weth ( 1 weth = 40000 DAI / 16 = 2500$) -> (Point 1)

Let's consider a pool which needs to be balanced with respect to other pools in the salty contracts i .e weth - dai pool 1 weth : 1 dai as you can see there are huge arbitrage profits to be made here. Alice sees the opportunity and calls depositSwapWithdraw with 34 dai. Lets look at the poc and look at its logs to get a better understanding of what happens during the swap procedure. You can run the poc by adding it to src/pools/tests/Pools.t.sol

COVERAGE="yes" NETWORK="sep" forge test --match-test testMagicNumber -vvvv --rpc-url "https://rpc.sepolia.org"

        function emitHelper() internal {
            (uint reserveWbtc_wbtc_weth_pool,uint reserveWeth_wbtc_weth_pool)= pools.getPoolReserves(wbtc,weth);
            (uint reserveWeth_weth_dai_pool,uint reserveDai_weth_dai_pool) = pools.getPoolReserves(weth,dai);
            (uint reserveWbtc_wbtc_dai_pool,uint reserveDai_wbtc_dai_pool) = pools.getPoolReserves(wbtc,dai);
            (uint reserveWeth_weth_salt_pool,uint reserveSalt_weth_salt_pool) = pools.getPoolReserves(weth,salt);
            (uint reserveSalt_salt_wbtc_pool,uint reserveWbtc_salt_wbtc_pool)  = pools.getPoolReserves(salt,wbtc);

            emit log_named_decimal_uint("reserve_Wbtc_wbtc_weth_pool", reserveWbtc_wbtc_weth_pool, 8); 
            emit log_named_decimal_uint("reserve_Weth_wbtc_weth_pool", reserveWeth_wbtc_weth_pool, 18);
            emit log_named_decimal_uint("reserve_Weth_weth_dai_pool", reserveWeth_weth_dai_pool, 18);
            emit log_named_decimal_uint("reserve_Dai_weth_dai_pool", reserveDai_weth_dai_pool, 18);
            emit log_named_decimal_uint("reserve_Wbtc_wbtc_dai_pool", reserveWbtc_wbtc_dai_pool, 8);
            emit log_named_decimal_uint("reserve_Dai_wbtc_dai_pool", reserveDai_wbtc_dai_pool, 18);
            emit log_named_decimal_uint("reserve_Weth_weth_salt_pool", reserveWeth_weth_salt_pool, 18);
            emit log_named_decimal_uint("reserve_Salt_weth_salt_pool", reserveSalt_weth_salt_pool, 18);
            emit log_named_decimal_uint("reserve_Salt_salt_wbtc_pool", reserveSalt_salt_wbtc_pool, 18);
            emit log_named_decimal_uint("reserve_Wbtc_salt_wbtc_pool", reserveWbtc_salt_wbtc_pool, 8);
        }

        function setupWethWbtcPool() internal {

            // let assume 1 
            vm.startPrank(DEPLOYER);
            weth.transfer(alice,16 ether);
            wbtc.transfer(alice,1e8);
            // alice deposits
            // 1 wbtc = 16 weth
            (uint wethBefore,uint wbtcBefore) = pools.getPoolReserves(weth,wbtc);
            assertEq(wethBefore,0);
            assertEq(wbtcBefore,0);
            changePrank(alice);
            wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
            weth.approve(address(collateralAndLiquidity), type(uint256).max);
            collateralAndLiquidity.depositCollateralAndIncreaseShare(1e8,16 ether,0,block.timestamp,false);
            vm.stopPrank();
        } 

        function setupDaiWbtcPool() internal {
            vm.startPrank(DEPLOYER);
            wbtc.transfer(address(alice),1e8);
            dai.transfer(address(alice), 40_000 ether);
            vm.stopPrank();
            vm.startPrank(address(alice));
            wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
            dai.approve(address(collateralAndLiquidity),type(uint256).max);
            (,,uint addedLiquidity)=collateralAndLiquidity.depositLiquidityAndIncreaseShare(wbtc,dai,1e8,40_000 ether,40_000 ether,block.timestamp,false);

            changePrank(bob);
            wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
            wbtc.approve(address(pools), type(uint256).max);
            dai.approve(address(pools),type(uint256).max);

            dai.approve(address(collateralAndLiquidity),type(uint256).max);
            vm.stopPrank();
        }
        function testMagicNumber() public {
            vm.label(address(dai),"dai");
            vm.label(address(weth),"weth");
            vm.label(address(wbtc),"wbtc");

            // setupWethDaiPool();
            setupDaiWbtcPool();
            setupWethWbtcPool();

            // making sure bob has nothing
            assertEq(dai.balanceOf(bob),0);
            assertEq(weth.balanceOf(bob),0);

            // now adding liq to a new pool, weth-dai
            // but bob frontruns 
            // 1st - 1 weth = 1 dai
            vm.startPrank(DEPLOYER);
            dai.transfer(bob,1 ether);
            weth.transfer(bob,1 ether);
            assertEq(dai.balanceOf(alice),0);
            assertEq(weth.balanceOf(alice),0);
            dai.transfer(alice,2500 ether);
            weth.transfer(alice, 1 ether);

            changePrank(bob);
            dai.approve(address(collateralAndLiquidity), type(uint).max);
            weth.approve(address(collateralAndLiquidity), type(uint).max);
           (,,uint addedBob) =collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth,dai,1 ether,1 ether,0,block.timestamp,false);

            uint magicNumber = 34 ether ;
           changePrank(alice);
            dai.approve(address(pools), type(uint).max);
            weth.approve(address(pools), type(uint).max);
           pools.depositSwapWithdraw(dai,weth,magicNumber,0,block.timestamp);
        //    pools.depositSwapWithdraw(dai,wbtc,magicNumber,0,block.timestamp);
            emit log_named_decimal_uint("arbProfit", pools.depositedUserBalance(address(dao),weth), 18);
           emitHelper();
        }

So the logs of poc are (ignoring the empty salt pools) - Logs: arbProfit: 0.000000000000000000 reserve_Wbtc_wbtc_weth_pool: 1.00000000 reserve_Weth_wbtc_weth_pool: 16.000000000000000000 reserve_Weth_weth_dai_pool: 0.028571428571428572 reserve_Dai_weth_dai_pool: 35.000000000000000000 reserve_Wbtc_wbtc_dai_pool: 1.00000000 reserve_Dai_wbtc_dai_pool: 40000.000000000000000000

So now according to DAI - WETH pool , ~0.029 weth = 35 dai, so 1 weth ~= 1207\$ which is not in sync with other pools by a 1293\$ (2500 - 1207). Even with this large arbitrage opportunity the atomic arbitrage fails to trigger leading to arbProfit =0, which in itself is a unexpected behaviour according to salty docs (close focus on point 2 & 3 in the doc).

So now let's see why the atomic arb failed to trigger in the swap call's _adjustReservesForSwapAndAttemptArbitrage first the reserves are adjusted to what shown in the logs above, but in the _attemptArbitrage call the valueInEth = 2.775e16 for 34 dai (according to the new adjustedReserves). which is double of what it should be hence following the

swap: swapTokenIn->WETH arb: WETH->DAI->WBTC->WETH

route will make the _bisectionSearch's return value to be 0 cause it will be calculated using a path that is unprofitable. Here in this path relatively large amt of weth will return a small amount of DAI, due to reserves not being in sync.

Making the amountOut variable < bestArbAmountIn.

Tools Used

Manual Review

Recommended Mitigation Steps

There should be at least two arbitrage path returned by the _arbitragePath. Two paths provide better gurantee that atomic arbitrage will be triggered For e.g-

In the coded POC if you can remove the comments from depositSwapWithdraw(dai,wbtc,...) and comment out depositSwapWithdraw(dai,weth,...) i.e make the last 6 lines like this

            weth.approve(address(pools), type(uint).max);
        //    pools.depositSwapWithdraw(dai,weth,magicNumber,0,block.timestamp);
           pools.depositSwapWithdraw(dai,wbtc,magicNumber,0,block.timestamp);
            emit log_named_decimal_uint("arbProfit", pools.depositedUserBalance(address(dao),weth), 18);
           emitHelper();
        }

and run it again this time you will see the pools are almost balanced and the DAO made a profit of whopping 0.65 weth.

This time the code works as expected because in the depositSwapWithdraw(dai,wbtc,...) call the path followed is WETH -> WBTC -> DAI -> WETH instead of WETH -> DAI -> WBTC -> WETH

Assessed type

Error

[c4-judge]

Picodes marked the issue as duplicate of #419

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

As mentioned in the Technical Overview, fixed arbitrage paths were used to keep gas costs as low as possible. By design the contracts do not try to extract all arbitrage opportunities:

https://tech.salty.io/technical-details/arbitrage

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

This report would be a better fit for the analysis as it's not strictly speaking a security issue but a design suggestion

[c4-judge]

Picodes marked the issue as grade-c