`CollSurplusPool.claimColl` doesn't respect `CEI` and could be drained if a reward token has hooks

[GalloDaSballo]

Impact

The function claimColl is as follows

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/CollSurplusPool.sol#L61-L74

  function claimColl(address _account) external override {
    _requireCallerIsProtocol();

    TokenAmount[] memory accountBalances = balances[_account];
    for (uint i = 0; i < accountBalances.length; i++) {
      TokenAmount memory tokenEntry = accountBalances[i];
      if (tokenEntry.amount == 0) continue;
      IERC20(tokenEntry.tokenAddress).transfer(_account, tokenEntry.amount); /// @audit Use SafeTransfer
    }

    delete balances[_account]; /// @audit Reentrancy / CEI
    emit CollClaimed(_account);
  }

It will iterate over a copy of the account balances and transfer them

Once done iterating it will delete the balances for the account

This breaks CEI, and if a token with hooks was added in the system it would allow draining of all balances by simply re-entering and calling claimColl multiple times

Mitigation

Adding nonReentrant could be a simple mitigation, but I believe you should instead re-think the function, as well as other part of the codebase to never break CEI

While this instance was identified, writing code that is not CEI compliant could result in a loss of funds, since the project is still in development, this is a good time to mitigate these type of findings via a safeByDefault refactoring

This is an untested refactoring:

  function claimColl(address _account) external override {
    // Check
    _requireCallerIsProtocol();

    // Effects
    // Save to memory
    TokenAmount[] memory accountBalances = balances[_account];
    // Delete from storage (prevents reentrancy below)
    delete balances[_account];

    // Interactions
    for (uint i = 0; i < accountBalances.length; i++) {
      TokenAmount memory tokenEntry = accountBalances[i];
      if (tokenEntry.amount == 0) continue;
      IERC20(tokenEntry.tokenAddress).transfer(_account, tokenEntry.amount); /// @audit Use SafeTransfer
    }
    emit CollClaimed(_account);
  }

[sambP]

covering that in #47