ctf_sec - More granular control of the pause is needed for each money market because deposit and withdrawal can be guaranteed to revert if the underlying money market is paused or has high utilization rate

[sherlock-admin]

ctf_sec

medium

More granular control of the pause is needed for each money market because deposit and withdrawal can be guaranteed to revert if the underlying money market is paused or has high utilization rate

Summary

More granular control of the pause is needed for each money market.

Vulnerability Detail

Currently a single pause control the stake and unstake function in UserManager.sol

    function stake(uint96 amount) public whenNotPaused nonReentrant {

and

    function unstake(uint96 amount) external whenNotPaused nonReentrant {

the stake function deposit fund into the AssetManager.sol and then deposit the fund into the money market to generate yield.

moneyMarket.deposit(token);

which calls

IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
uint256 amount = token.balanceOf(address(this));
lendingPool.deposit(tokenAddress, amount, address(this), 0);

The unstake withdraw fund from the AssetManager.sol, which withdraw money from the money market to return the user fund.

lendingPool.withdraw(tokenAddress, tokenAmount, recipient);

however, the AaveV3Adapter.sol#deposit and withdraw can revert if the underlying pool is paused or have high utilization rate.

The logic from AAVE did the check when depositing

https://github.com/aave/aave-v3-core/blob/f3e037b3638e3b7c98f0c09c56c5efde54f7c5d2/contracts/protocol/libraries/logic/ValidationLogic.sol#L57

  function validateSupply(DataTypes.ReserveCache memory reserveCache, uint256 amount)
    internal
    view
  {
    require(amount != 0, Errors.INVALID_AMOUNT);

    (bool isActive, bool isFrozen, , , bool isPaused) = reserveCache
      .reserveConfiguration
      .getFlags();
    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
    require(!isFrozen, Errors.RESERVE_FROZEN);

and check before withdraw.

https://github.com/aave/aave-v3-core/blob/f3e037b3638e3b7c98f0c09c56c5efde54f7c5d2/contracts/protocol/libraries/logic/ValidationLogic.sol#L87

   function validateWithdraw(
    DataTypes.ReserveCache memory reserveCache,
    uint256 amount,
    uint256 userBalance
  ) internal pure {
    require(amount != 0, Errors.INVALID_AMOUNT);
    require(amount <= userBalance, Errors.NOT_ENOUGH_AVAILABLE_USER_BALANCE);

    (bool isActive, , , , bool isPaused) = reserveCache.reserveConfiguration.getFlags();
    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
  }

as we can see,

    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
    require(!isFrozen, Errors.RESERVE_FROZEN);

Impact

Deposit and withdrawal can be guaranteed to revert if the underlying money market is paused or has high utilization rate

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/Controller.sol#L147-L151

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AaveV3Adapter.sol#L199-L204

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AaveV3Adapter.sol#L212-L219

Tool used

Manual Review

Recommendation

More granular control of the pause is needed for each money market. When we know the deposit / withdrawal is guaranteed to revert, we can skip the money market when depositing.

For example, we can add isPaused function in AaveV3Adapter.sol

function isPaused() external returns (bool) {
  return _poolPaused();
}

for _poolPaused(), we can check if the underlying Aave Pool is paused.

https://github.com/aave/aave-v3-core/blob/f3e037b3638e3b7c98f0c09c56c5efde54f7c5d2/contracts/misc/AaveProtocolDataProvider.sol#L155

  /**
   * @notice Returns if the pool is paused
   * @param asset The address of the underlying asset of the reserve
   * @return isPaused True if the pool is paused, false otherwise
   **/
  function getPaused(address asset) external view returns (bool isPaused) {
    (, , , , isPaused) = IPool(ADDRESSES_PROVIDER.getPool()).getConfiguration(asset).getFlags();
  }

For consistency reason, we can add isPaused() in PureTokenAdapter.sol, we never need to pause the PureTokenAdapter.sol

function isPaused() external returns (bool) {
  return false;
}

then when depositing or withdraw, we can change from

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L286-L290

to

    if (!moneyMarket.supportsToken(token)) continue;
    if (moneyMarket.floorMap(token) <= moneyMarket.getSupply(token)) continue;
    
    if(moneyMarket.isPaused()) continue;

    poolToken.safeTransferFrom(msg.sender, address(moneyMarket), amount);
    moneyMarket.deposit(token);

and in case of the deposit / withdraw fail because the underlying market has high utilization rate,

we can use try catch to wrap the deposit / withdraw.

   try (moneyMarket.deposit(token)) return (bool result) { 
         remaining = False;
   }  catch {

   }

Duplicate of #37

[kingjacob]

Dupe of #37

[kingjacob]

Agree but this is a dupe.