[IPool] - gulp should check if the token exists before updating state and executing outside code

[cleanunicorn]

Description

If one of the users sends tokens directly to the contract and wants the pool to update its internal balance, they can call the publicly available method gulp. This method first calls balanceOf for the provided token of the pool.

  function gulp(address token) external _lock_ {
    Record memory record = _records[token];
    uint256 balance = IERC20(token).balanceOf(address(this));

The balanceOf method of the provided token address is executed whether the token exists in the pool or not. This is because _records is defined as a storage mapping.

  // Internal records of the pool's underlying tokens
  mapping(address => Record) internal _records;

This means that any user can provide any contract's address, which contains a balanceOf method, which will be executed by the IPool contract. Because the token is not bound, the execution follows the else branch in gulp.

    Record memory record = _records[token];
    uint256 balance = IERC20(token).balanceOf(address(this));
    if (record.bound) {
[...]
    } else {
      _pushUnderlying(token, address(_unbindHandler), balance);
      _unbindHandler.handleUnbindToken(token, balance);
    }

Not only the contract executes external code, but it also calls _unbindHandler.handleUnbindToken(token, balance); which, in its current implementation, emits an event which could potentially be malicious when parsed by the frontend or the backend.

  /**
   * @dev Receive `amount` of `token` from the pool.
   */
  function handleUnbindToken(address token, uint256 amount)
    external
  {
    require(msg.sender == address(_pool), "ERR_ONLY_POOL");
    emit NewTokensToSell(token, amount);
  }

Recommendation

Only accept valid tokens

Check the validity of the provided address before executing any external code or emitting any events.

Static call balanceOf

While presenting the current issue to Dillon, a good suggestion was surfaced to do a static balanceOf call.

However, after checking the generated assembly, we realized Solidity already uses STATICCALL when calling view functions. This is also described in the official documentation.