2-day audit of Peapods Strategies with integrations with Balancer and UniswapV2

popcorn_audit_0701.md

[M-01] Peapods Vault harvesting prone to sandwich attacks due to use of balance in slippage protection

When harvesting rewards in Peapods Vaults, the claimed $PEAS rewards are sold for tokens that would be added as liquidity to a UniswapV2 or Balancer pool. The LP Tokens received are then checked against minOut to ensure that that there was no excessive slippage.

However, minOut is compared against the Peapods Vault's balance of the LP token instead of just the LP tokens received from adding liquidity to the Pool.

ref: BaseUinV2LpCompounder::sellRewardsForLpTokenViaUniswap()

  uint256 amountLP = IERC20(vaultAsset).balanceOf(address(this));
  uint256 minOut = abi.decode(data, (uint256));
  if (amountLP < minOut) revert CompoundFailed();

There are two situations when the Vault would have a floating balance of LP tokens:

1. The owner or keeper has pulled the LP tokens from Peapods using pullFunds().
2. The Vault does not autodeposit the LP tokens into Peapods when a user deposits the LP tokens into the Vault.

This behavior can be tested with the following changes to the PeapodsUniV2CompounderTest test file.

@@ -223,14 +258,16 @@ contract PeapodsUniV2CompounderTest is BaseStrategyTest {
         deal(
             address(0x02f92800F57BCD74066F5709F1Daa1A4302Df875),
             address(strategy),
-            10 ether
+            1 ether
         );
 
-        uint256 totAssetsBefore = strategy.totalAssets();
+        uint256 totAssetsBefore = strategy.totalAssets();        
+        strategy.pullFunds(totAssetsBefore, "");
 
-        strategy.harvest(abi.encode(0));
+        strategy.harvest(abi.encode(12094524737339));
 
         // total assets have increased
+        console.log("Gained assets: ", strategy.totalAssets() - totAssetsBefore);
         assertGt(strategy.totalAssets(), totAssetsBefore);
     }

Run the test script with:

forge test --m test__harvest --mc PeapodsUniV2CompounderTest -vvv

The test shows that the harvest() succeeds even though the gained assets of 12094524737338 is less than the minOut of 12094524737339.

The slippage protection is ineffective in this case and leaves the harvest() vulnerable to sandwich attacks. Considering that the PEAS pools like the PEAS/WETH pool have low liquidity, sandwich attacks will be more profitable for attackers.

Recommendation:

Consider using the delta of the LP Token balance before and after adding liquidity to the pool to get the actual amount of LP tokens received. The code may look like:

uint256 preBalance = IERC20(vaultAsset).balanceOf(address(this));
UniswapV2TradeLibrary.addLiquidity(
    uniswapRouter, depositAssets[0], depositAssets[1], amountA, amountB, 0, 0, to, deadline
);

uint256 amountLP = IERC20(vaultAsset).balanceOf(address(this)) - preBalance;
uint256 minOut = abi.decode(data, (uint256));
if (amountLP < minOut) revert CompoundFailed();

The fix would need to be applied to sellRewardsForLpTokenViaUniswap() in BaseUniV2LpCompounder and to the sellRewardsForLpTokenViaBalancer() in BaseBalancerLpCompounder.

[L-01] Peapod Base Strategy initialize will not work for ERC20s that do not return bool for approve()

There is no impact in PeapodsBalancerUniV2Compounder and PeapodsUniV2Compounder, since those use either Balancer or UniswapV2 LP tokens. However, if the asset is an ERC20 that does not return a boolean for the approve() function like USDT, initialize will always revert.

IERC20(asset_).approve(staking_, type(uint256).max);

Recommendation:

Consider using SafeERC20.forceApprove() instead in __PeapodsBase_init().

[L-02] Approvals are not voided when Uniswap Router is changed

The harvesting details can be changed by the owner of the Peapods UniswapV2 Strategy. When changing the harvesting details, the new Uniswap Router is set before the old token approvals are voided.

ref: BaseUniV2LpCompounder::setUniswapLpCompounderValues()

setUniswapTradeValues(newRouter, rewardTokens, newSwaps);

address tokenA = newDepositAssets[0];
address tokenB = newDepositAssets[1];

address oldTokenA = depositAssets[0];
address oldTokenB = depositAssets[1];

if (oldTokenA != address(0)) {
    IERC20(oldTokenA).forceApprove(address(uniswapRouter), 0);
}
if (oldTokenB != address(0)) {
    IERC20(oldTokenB).forceApprove(address(uniswapRouter), 0);
}

Since uniswapRouter has changed by the time the old approvals are voided, the old approvals remain in the previous uniswapRouter. There is no impact but it is worth removing unnecessary approvals as a security measure.

Recommendation:

Consider voiding the old approvals before setting the new Uniswap Router. The issue also exists in BaseBalancerLpCompounder::setBalancerLpCompounderValues().

[L-03] Parent contracts have no Storage Gaps

The Peapods Strategy contracts are upgradeable and inherit from parent contracts such as BaseUniV2LpCompounder and BaseUniV2Compounder. However, these parent contracts do not use any storage gaps. Adding new state variables to any of these parent contracts will shift down the state variables of its children contracts. This can lead to data corruption.

Recommendation:

Consider implementing storage gaps for all parent contracts that have not yet been deployed or document the risk of adding new state variables to any of the parent contracts.

Personal notes:

• UniswapV2 integration was easier to audit because there's only one type of Pool (UniswapV2Pair) and the router is simple.
• Balancer was more difficult because it had several types of Pools and its other pools were more complicated than the typical 2-token pool.
• The strategies only expect to work with ERC20s and will not handle any rewards/assets in ETH. Checked and it looks like none of the rewards are in ETH.