QA Report

[code423n4]

• 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.

[PierrickGT]

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

Duplicate of #11

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

Duplicate of #4

Not sure if the error message is correct here:

It fits in 32 bytes and inform that the allowance for aToken can't be changed.

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

We usually use from instead of caller or sender, so it makes sense to use it here too.

decimals() is not used in any meaningful way

As stated in a previous issue, we set the decimals value in our deploy script.

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.

It would add a new function and some logic that is not necessary. Users can know their balance before calling the redeemToken function by calling balanceOfToken.

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

It would be a redundant check and there is no financial incentive to run these kind of transactions.

safeApprove is deprecated

Deprecated but still safe to use to approve from a 0 allowance. It will also return a useful error message if approving from a non zero allowance.