PoolTogether Aave v3 contest - pauliax's results

• The contract is compiled with the version that has safe math enabled by default, yet it still explicitly uses the library:

  pragma solidity 0.8.10;
  using SafeMath for uint256;   
  uint256 _balanceDiff = _afterBalance.sub(_beforeBalance);
  return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
  return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply);

• Would be better to re-use _requireNotAToken() function here:

    function transferERC20(
      ...
      require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");

• Not sure if the error message is correct here:

    require(_token != address(aToken), "AaveV3YS/forbid-aToken-allowance");

It is a bit misleading to name the parameter 'from' here:

    event DecreasedERC20Allowance(
        address indexed from,
    event IncreasedERC20Allowance(
        address indexed from

because the actual allowance is increased/decreased from the contract itself. A more intuitive name would be a 'sender' or 'caller', or something like that.

• decimals() is not used in any meaningful way. A comment says: "This value should be equal to the decimals of the token used to deposit into the pool." so I think you can at least query aToken.UNDERLYING_ASSET_ADDRESS().decimals() in the constructor to ensure that the decimals match.

• This might not be compatible with IYieldSource, but I think it would be helpful to have an extra function redeemShares, so that users can specify their balance of shares directly when redeeming.

• function redeemToken could validate that _redeemAmount > 0 to prevent spam of useless invocations.

• safeApprove is deprecated: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L45

  // Approve once for max amount
  IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);

I think you can just use a regular 'approve' in the constructor to set the initial approval.

• I think the value of UNDERLYING_ASSET_ADDRESS can't change so you could cache it in the constructor to avoid repetetive external calls:

    function _tokenAddress() internal view returns (address) {
        return aToken.UNDERLYING_ASSET_ADDRESS();
    }

especially considering that supply and redeem relies that the underlying token does not change.

• I do not think AaveV3YieldSourceInitialized needs an indexed parameter because this event is only emitted once, in the constructor.