QA Report

[code423n4]

Table of Contents:

• [L-01] Add a timelock to transferERC20
• [L-02] Prevent accidentally burning tokens
• [L-03] Deprecated safeApprove() function
• [N-01] Missing comment section saying "public" instead of "external"

[L-01] Add a timelock to transferERC20

A Malicious manager or owner could transfer any amount of token to any address.

File: AaveV3YieldSource.sol
332:   function transferERC20(
333:     IERC20 _token,
334:     address _to,
335:     uint256 _amount
336:   ) external onlyManagerOrOwner { //@audit: timelock needed here
337:     require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");
338:     _token.safeTransfer(_to, _amount);
339:     emit TransferredERC20(msg.sender, _to, _amount, _token);
340:   }

To give more trust to users: this function should be put behind a timelock.

[L-02] Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

File: AaveV3YieldSource.sol
332:   function transferERC20(
333:     IERC20 _token,
334:     address _to,
335:     uint256 _amount
336:   ) external onlyManagerOrOwner {
337:     require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");
338:     _token.safeTransfer(_to, _amount); //@audit to should be address(0) checked
339:     emit TransferredERC20(msg.sender, _to, _amount, _token);
340:   }

[L-03] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

File: AaveV3YieldSource.sol
182:     // Approve once for max amount
183:     IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);

[N-01] Missing comment section saying "public" instead of "external"

The following functions are under the external section:

File: AaveV3YieldSource.sol
196:   /* ============ External Functions ============ */
...
207:   /**
208:    * @notice Returns the ERC20 asset token used for deposits.
209:    * @return The ERC20 asset token address.
210:    */
211:   function depositToken() public view override returns (address) { //@audit should be under /* ============ Public Functions ============ */
212:     return _tokenAddress();
213:   }
214: 
215:   /**
216:    * @notice Returns the Yield Source ERC20 token decimals.
217:    * @dev This value should be equal to the decimals of the token used to deposit into the pool.
218:    * @return The number of decimals.
219:    */
220:   function decimals() public view virtual override returns (uint8) { //@audit should be under /* ============ Public Functions ============ */
221:     return _decimals;
222:   }

Consider adding a comment mentioning the public section: /* ============ Public Functions ============ */.

[PierrickGT]

[L-01] Add a timelock to transferERC20

This is an emergency function that should be used only if some ERC20 token were mistakenly sent to the yield source. Implementing a timelock seems to be an over engineered solution.

[L-02] Prevent accidentally burning tokens

We may want to burn some tokens that have no values and were sent only to promote a scammy project.

[L-03] Deprecated safeApprove() function

Duplicate of #39

[N-01] Missing comment section saying "public" instead of "external"

Public functions are part of the external ones, seems redundant to add a public section.