Platform: Code4rena
Start Date: 17/06/2021
Pot Size: $60,000 USDC
Total HM: 12
Participants: 12
Period: 7 days
Judge: LSDan
Total Solo HM: 8
Id: 14
League: ETH
Rank: 5/12
Findings: 3
Award: $2,502.87
🌟 Selected for report: 5
🚀 Solo Findings: 0
pauliax
_depositInVault in contract YearnV2YieldSource calls safeApprove when the allowance is less than the token balance: if (token.allowance(address(this), address(v)) < token.balanceOf(address(this))) { token.safeApprove(address(v), type(uint256).max); } This does not mean that the current allowance is 0. safeApprove usage: "safeApprove should only be called when setting an initial allowance, or when resetting it to zero. To increase and decrease it, use 'safeIncreaseAllowance' and 'safeDecreaseAllowance'". Reference: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/utils/SafeERC20Upgradeable.sol So this can theoretically start failing once the approval is higher than 0 but lower than the token balance. In practice this will probably never be triggered cuz the initialize function has initially approved the max amount. Rating this as low as the possibility is very unlikely but the impact could be huge (won't be able to deposit tokens in vault).
By the way, overall safeApprove is deprecated, so you may consider replacing it with safeIncreaseAllowance/safeDecreaseAllowance:
Solution: token.safeApprove(address(v), 0); token.safeApprove(address(v), type(uint256).max); Or completely remove this check to save some gas as the max amount is already approved in the initialize function.
#0 - asselstine
2021-06-26T18:48:58Z
#1 - dmvt
2021-07-31T17:56:57Z
duplicate of #71
pauliax
I expect to see nonReentrant on functions with external calls (e.g. moving tokens). Here is a list (it may not be complete) of functions that do not have this modifier: Contract SushiYieldSource functions supplyTokenTo, redeemToken. Contract IdleYieldSource function sponsor. Contract BadgerYieldSource functions supplyTokenTo, redeemToken. Contract ATokenYieldSource function sponsor.
Add nonReentrant modifier to the functions that interact with external actors.
#0 - asselstine
2021-06-24T23:05:14Z
#1 - dmvt
2021-07-31T18:00:57Z
dropping this to a level 1 since no threat is actually defined and the other duplicate issues all agree this is a low risk
#2 - dmvt
2021-07-31T18:01:17Z
duplicate of #119
🌟 Selected for report: pauliax
300.9419 USDC - $300.94
pauliax
Here it queries _vault.apiVersion() 3 times, it would be more efficient to query it once and store in a variable which can be used later to avoid these extra external calls: require(!areEqualStrings(_vault.apiVersion(), "0.3.2"), "YearnV2YieldSource:: vault not compatible"); require(!areEqualStrings(_vault.apiVersion(), "0.3.3"), "YearnV2YieldSource:: vault not compatible"); require(!areEqualStrings(_vault.apiVersion(), "0.3.4"), "YearnV2YieldSource:: vault not compatible");
#0 - PierrickGT
2021-07-01T14:44:36Z
Corrected in this PR: https://github.com/pooltogether/pooltogether-yearnv2-yield-source/pull/8
135.4239 USDC - $135.42
pauliax
Some yield sources approve max amount initially (function initialize), some approve the exact amount on deposit action. It would reduce some gas if all yield sources approved max amount once (on init).
Approve max amount of yield source underlying asset on init.
#0 - asselstine
2021-06-26T17:53:51Z
🌟 Selected for report: pauliax
300.9419 USDC - $300.94
pauliax
function _accrueCredit just calls _updateCreditBalance. All the calls to _accrueCredit can be replaced with _updateCreditBalance to save some gas that incurs when invoking a function that just forwards the job to another function.
Replace calls to _accrueCredit with _updateCreditBalance.
#0 - asselstine
2021-06-26T18:35:39Z
It function adds a trivial amount of gas (for a JUMP). It really helps readability, so we'll keep it.
🌟 Selected for report: pauliax
300.9419 USDC - $300.94
pauliax
modifier canAddLiquidity calls internal function _canAddLiquidity. This function is not called anywhere else so I do not see a reason why all the logic can't be moved to the modifier to save some gas by reducing the extra call.
Remove function _canAddLiquidity, place its logic directly in the canAddLiquidity modifier.
#0 - asselstine
2021-06-26T18:33:09Z
I believe our intention was to have an external function to check; so in fact the fix here would be to add the external function
canAddLiquidity
135.4239 USDC - $135.42
pauliax
Here when the balances are equal it emits CreditBurned event: if (oldBalance < newBalance) { emit CreditMinted(user, controlledToken, newBalance.sub(oldBalance)); } else { emit CreditBurned(user, controlledToken, oldBalance.sub(newBalance)); } It would be more precise not to emit the event at all when the balance does not change.
if (oldBalance < newBalance) { emit CreditMinted(user, controlledToken, newBalance.sub(oldBalance)); } else if (oldBalance > newBalance) { emit CreditBurned(user, controlledToken, oldBalance.sub(newBalance)); }
#0 - asselstine
2021-06-25T22:44:45Z
#1 - dmvt
2021-07-31T17:54:53Z
duplicate of #97
135.4239 USDC - $135.42
pauliax
contract ATokenYieldSource function _depositToAave returns 0 if successful. However, this value is not checked nor used anywhere. As this function is internal it would probably be better to remove this unnecessary return to save some gas and eliminate confusion.
refactor function _depositToAave to return void.
#0 - PierrickGT
2021-06-28T12:52:14Z
🌟 Selected for report: pauliax
300.9419 USDC - $300.94
pauliax
function _getRefferalCode() in ATokenYieldSource just returns a constant of uint16(188). To save some gas and improve the readability this can be extracted to a constant variable and used where necessary.
uint16 internal constant REFFERAL_CODE = uint16(188);
#0 - PierrickGT
2021-07-07T12:48:46Z
@asselstine do we want to also add the ability to set a different referral code?
If we extract it in a constant, we may as well also assign addressesProviderId to a constant: https://github.com/pooltogether/aave-yield-source/blob/efa84639041ae7feaa73b9c8ddc18bf2d5d6c180/contracts/yield-source/ATokenYieldSource.sol#L260
#1 - PierrickGT
2021-07-07T15:04:53Z