0xTheC0der - LiquidationRow.liquidateVaultsForToken(...) will always revert due to missing token transfers

[sherlock-admin2]

0xTheC0der

high

LiquidationRow.liquidateVaultsForToken(...) will always revert due to missing token transfers

Summary

The LiquidationRow.liquidateVaultsForToken(...) method, which is a core functionality of the protocol, will revert in any case due to missing token transfers to and from the async swapper making the protocol unable to liquidate rewards from the destination vaults, swap them & forward them to the main rewarder, i.e. loss of assets for protocol/users.

Vulnerability Detail

Contract BaseAsyncSwapper is IAsyncSwapper

The swap(...) method requires that the sellAmount of sellToken was already transferred to the contract, see L28-32, before the swap(...) method is callled in order to succeed. Otherwise it will revert with InsufficientBalance.
Furthermore, the swapped buyTokenAmountReceived of buyToken is never transferred back to the msg.sender and therefore irrecoverably stuck.
At this point it looks like this contract intended to be used via delegatecall, but in fact it is not used that way.

See test_swap() in OneInchAdapter.t.sol and ZeroExAdapter.t.sol for correct usage of swap(...) by transferring tokens beforehand. (Swapped tokens are still stuck, but this is not covered by the test cases.)

Side note: An excess amount of sellToken is not refunded by the swap method. Furthmore, if the sellToken were transferred in a separate transaction, they become stuck on swap failure. However, this is not the focus of this report.

Contract LiquidationRow

The liquidateVaultsForToken(...) method, which liquidates the fromToken using an async swapper (see above), subsequently relies on _performLiquidation(...) which performs the async swap in L251.
However, here the IAsyncSwapper.swap(...) method is just called "normally" and not via delegatecall. In addition, the fromToken are not transferred to the async swapper, therefore liquidateVaultsForToken(...) will always revert with IAsyncSwapper.InsufficientBalance and no liquidations are possible in any case.

Side note: This cannot be circumvented by transferring the fromToken to the async swapper directly before calling liquidateVaultsForToken(...) since the swapped buyToken will still be stuck in the async swapper.

Use case LiquidationRow.t.sol

Just to provide further clarification for the reader: The test cases in the LiquidateVaultsForToken contract show that the LiquidationRow contract (and not the asyc swapper) is expected to hold the fromToken, see L213. This again confirms the issue that a token transfer is missing in LiquidationRow._performLiquidation(...) before initiating the swap.

Yes, but why are the test cases passing then?
The test cases use an AsyncSwapperMock contract which overrides the BaseAsyncSwapper.swap(...) method and therfore does not require an inbound tansfer of sellToken and replaces the missing outbound transfer of buyToken with a simple token mint to the LiquidationRow contract.

Impact

The LiquidationRow.liquidateVaultsForToken(...) method, which is a core functionality of the protocol and intended to be invoked via the sponsor's automated off-chain components, will always revert.

As a consequence, the mechanism to liquidate vault rewards into another asset such as WETH, see README, is not available and therefore resulting in a loss of funds for the protocol/users due to not being able to liquidate claimed rewards from the destination vaults, swap them & forward them to the main rewarder. As a further consequence of this, destination vault rewards claimed via LiquidationRow.claimsVaultRewards(...) are then permanently stuck in the contract.

Code Snippet

The following PoC modifies the LiquidationRow test in a way that the AsyncSwapperMock behaves more like the real BaseAsyncSwapper. As a result, the LiquidationRow.liquidateVaultsForToken(...) method fails in any case and therefore proves the above claims.

Just apply the diff below and run the tests with forge test -vv --match-contract LiquidateVaultsForToken:

diff --git a/test/liquidators/LiquidationRow.t.sol b/test/liquidators/LiquidationRow.t.sol
index 5d4955e..2a6569c 100644
--- a/test/liquidators/LiquidationRow.t.sol
+++ b/test/liquidators/LiquidationRow.t.sol
@@ -44,6 +44,16 @@ contract AsyncSwapperMock is BaseAsyncSwapper {
     }
 
     function swap(SwapParams memory params) public override returns (uint256 buyTokenAmountReceived) {
+        // ------------------------>
+        // copied from BaseAsyncSwapper.swap(...)
+        IERC20 sellToken = IERC20(params.sellTokenAddress);
+        uint256 sellTokenBalance = sellToken.balanceOf(address(this));
+
+        if (sellTokenBalance < params.sellAmount) {
+            revert InsufficientBalance(sellTokenBalance, params.sellAmount);
+        }
+        // <------------------------
+
         targetToken.mint(liquidationRow, params.sellAmount);
         return params.sellAmount;
     }

Tool used

Manual Review

Recommendation

1. Carefully consider using IAsyncSwapper.swap(...) via delegatecall since it seems intended to be used this way.
2. Or fix the missing tansfer of sellToken/fromToken from the LiquidationRow contract to the async swapper and the missing transfer of buyToken from the async swapper to the LiquidationRow contract.
   In this case, make sure that excess sellToken are refunded and all sellToken are refunded on swap failure.

Duplicate of #205