Euler

[vany365]

Description:

Commit (hash):

Tag:

Repo: https://github.com/OpenOrg-gg/yearn-euler

Scope: (List link to files)

• File.sol

Due Diligence: (attach link to due diligence document, see template here)

Additional References or Repo (Link to underlying protocols or relevant documents):

Deployed Contract (etherscan link):

Review Ongoing By:

• [ ] @dudesahn
• [x] @newmickymousse

Review Completed By:

• [ ] @dudesahn
• [ ] @newmickymousse

[newmickymousse]

1. https://github.com/OpenOrg-gg/yearn-euler/blob/master/contracts/Strategy.sol#L213-L230: this method should be access by vaultManagement and not necessarily onlyGov because if not we will have to send a CMO tx every time we claim. We have to check a few things here: 1) if it is really necessary that the smart contract claims or if anyone can claim on your behalf and you will receive the rewards anyways. Balancer was like this and we did the claims weekly but without having a method inside the contract.
2. Let's use a variable that we can set as keepEul to decide how much we are going to keep every time we harvest/claim instead of denominator and numerator.
3. In liquidatePosition (https://github.com/OpenOrg-gg/yearn-euler/blob/master/contracts/Strategy.sol#L232-L254) first check if balanceOfWant() is enough to cover _amountNeeded.
4. Use balanceOfWant() in Lines 137, 260
5. What happens if there is not enough liquidity to pool our funds? How does liquidatePosition manages this?
6. Given that we can deploy in different vaults, can we make it clonable as well?
7. Can this strategy become a gen lender plugin? We can still manage the claims from outside and claim before harvest.
8. On prepareMigration, we can also migrate existing eulTokens in the strategy.

[OpenOrg-gg]

1. Done

2. Done.

3. Unclear - follow up question in tg.

4. Done.

5. on Line 239 we check if there is enough, if so we withdraw that amount, otherwise we withdraw the max available and then check the balance. If it was a lack in liquidity we would temporarily count it as a loss, and then recount it as a profit once it was actually available again.

6. Yes - need to discuss this.

7. No as discussed in tg.

8. Didn't add, as some projects once you can claim still have transfers not allowed to start, didn't want to have prepareMigration revert if that's the case and haven't seen a clean standard way to safely check transferability.

[dudesahn]

1. Recommend grouping view functions together to make the code easier to follow
2. What is the subAccountId in withdraw()? Will it always be 0 or should this be adjustable or dynamic?
3. I would add a check in adjustPosition to return if emergency exit is true.
4. What's the point of this check? Won't the deposit just revert if the market has changed? https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L206
5. Stylistically, I prefer using if statements instead of the ? : on L156, L168—imo makes the code easier to follow.
6. https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L167 Why are you adding to loss here? isn't it guaranteed to start at zero?
7. Can funds get stuck in Euler? For instance, through utilization? If so, I think your accounting may get messed up because of how you're calculated profit/loss.
8. https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L182 is this function really needed? just seems like a wrapper for eToken.withdraw(0,_amount);, I would just use that where it's needed.
9. Will this revert if trying to deposit 0? I generally check to make sure amount is greater than 0 anyway so we don't waste gas. https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L209
10. Same thing Micky said— you should be checking that you don't have enough free balance loose in the strategy. Something like this:

        uint256 _wantBal = balanceOfWant();
        if (_amountNeeded > _wantBal) {
            uint256 _stakedBal = stakedBalance();
            if (_stakedBal > 0) {
                rewardsContract.withdraw(
                    Math.min(_stakedBal, _amountNeeded.sub(_wantBal)));
            }
            uint256 _withdrawnBal = balanceOfWant();
            _liquidatedAmount = Math.min(_amountNeeded, _withdrawnBal);
            _loss = _amountNeeded.sub(_liquidatedAmount);
        } else {
            // we have enough balance to cover the liquidation available
            return (_amountNeeded, 0);
        }

11. Again, does Euler allow for funds to be locked up? https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L254
12. Probably good to add a check to make sure the token amount is greater than 0: https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L266
13. Are you planning to use this? https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L312
14. Would make this at least vaultManagersOnly, if not onlyGov: https://github.com/OpenOrg-gg/yearn-euler/blob/05f405e5cf9fdf82b89bd4dd5444cf44beb9a906/contracts/Strategy.sol#L340
15. No harvestTrigger? 🥲

[OpenOrg-gg]

Changes added on branch "Feedback 1.2" - bolded points that need more feedback.

1. Noted

2. Should always be 0, every address can have multiple subaccounts they set up to isolate positions. 0 is the master account.

3. Unsure about the purpose on this. Is this just a flag that if=true we do not redeposit?

4. In theory the market could be updated to a new version to support the same asset, and we'd be depositing into the old market which will still exist but just have 0 usage.

5. I've pulled this from previous changes Micky added in saying that this way was cleaner. Happy to revert but will let you two debate.

6. Under normal operations, but in the event of issues with the market or a hack then you could be at a non-zero value here, right?

7. They can - my understanding was the practice was if funds are stuck count them as a loss, then if they ever get unstuck count them as a profit later?

8. No, but I was told it was cleaner to create functions for any calls we're using more than once?

9. Good point. Fixed.

10. Ok think I have this revised.

11. Yes, will follow up to discuss.

12. Would we trigger the prepareMigration() call on a strategy if there was 0? Thought that is a manual call?

13. No, it came in the standard mix, and I thought it would be required? I can remove it if not needed.

14. Set to only Governance.

15. Claims aren't live and need to be done with a proof off chain, so no need for harvest trigger.

[dudesahn]

Holy shit, it's seriously been a week? Good god. 😵‍💫

3. yes, exactly
4. ugh okay leave it for now, I'll go wage war with the mouse
5. I'm saying there's nothing to add to it. as in, _loss is already zero, because you don't assign it any value anywhere above in prepareReturn
6. I think counting stuck funds as a loss is bad practice—instead, I would return partial withdrawals and not assess losses (so you would only get a partial burning of shares for anyone who hits the limit).
7. meh, I guess
8. You might still, for instance if you've already returned all assets to the vault but want to keep all of the gain/loss data for the strategy. But I don't need to die on that hill.
9. Nah, you can just remove that return line and have it be a blank function
10. Makes sense

[OpenOrg-gg]

Holy shit, it's seriously been a week? Good god. 😵‍💫

3. yes, exactly

Done

5. I'm saying there's nothing to add to it. as in, _loss is already zero, because you don't assign it any value anywhere above in prepareReturn

But this is calculating the loss and adding it in right? The line is if the debt of the vault is more than the assets we have, there must be a loss. That loss must be the delta between the two values.

So it starts at 0 but we're adding _vaultDebt.sub(_totalAssets) if vaultDebt > totalAssets. Otherwise we keep it at 0?

6. I think counting stuck funds as a loss is bad practice—instead, I would return partial withdrawals and not assess losses (so you would only get a partial burning of shares for anyone who hits the limit).

I'm not sure how I would structure that; but, I also think I disagree.

Stuck funds are extremely rare and usually only in a problem state, especially with a new protocol that hasn't been around as long as other lending markets like Compound.

I think at early stages it is most likely that stuck funds represent an exploit, or a breakdown of the protocol where the ability to return those assets is uncertain. I think they should be counted as lost until they can be returned in this case.

Otherwise the assets are stuck if they are not counted as a loss, a user will be pulling funds from other strategies in the vault, without us being sure if we will recoup those assets.

It feels like this is something that should be handled on a per protocol basis, based on what can cause stuck funds, and how uncommon it is?

7. meh, I guess
8. You might still, for instance if you've already returned all assets to the vault but want to keep all of the gain/loss data for the strategy. But I don't need to die on that hill.
9. Nah, you can just remove that return line and have it be a blank function
10. Makes sense

Other changes are on this PR and I'll keep it rolling with the changes so they are easier to find:

https://github.com/OpenOrg-gg/yearn-euler/pull/3

[saltyfacu]

hey @OpenOrg-gg ! how's it going with the fixes?

[dudesahn]

But this is calculating the loss and adding it in right? The line is if the debt of the vault is more than the assets we have, there must be a loss. That loss must be the delta between the two values.

So it starts at 0 but we're adding _vaultDebt.sub(_totalAssets) if vaultDebt > totalAssets. Otherwise we keep it at 0?

I'm saying the code should instead just be:

_loss =   _vaultDebt > _totalAssets ? _vaultDebt.sub(_totalAssets) : 0;

[dudesahn]

hey @OpenOrg-gg ! how's it going with the fixes?

I'm back on it, my fault, will stay on this until it's done :)

[dudesahn]

I'm not sure how I would structure that; but, I also think I disagree.

Stuck funds are extremely rare and usually only in a problem state, especially with a new protocol that hasn't been around as long as other lending markets like Compound.

I think at early stages it is most likely that stuck funds represent an exploit, or a breakdown of the protocol where the ability to return those assets is uncertain. I think they should be counted as lost until they can be returned in this case.

Otherwise the assets are stuck if they are not counted as a loss, a user will be pulling funds from other strategies in the vault, without us being sure if we will recoup those assets.

It feels like this is something that should be handled on a per protocol basis, based on what can cause stuck funds, and how uncommon it is?

For gen lender, we handle stuck funds as non-lossy: https://github.com/flashfish0x/yearnV2-generic-lender-strat/blob/f55c63ef3feaa03fb61e262ed4954db3e7f1cddc/contracts/Strategy.sol#L521

yes, it would pass on to other strategies, but I also don't really like the idea of passing on a loss to a user who happens to hit the lender in the queue if utilization is at 100%. it has happened multiple times that we get 100% utilization in lenders, or at the very least that yearn has more funds deposited into a lender than is liquid in the market itself.

In the situation where losses aren't assessed, and the user just gets back only free funds, that prevents users from taking the loss individually (or assessing it to the whole vault) but also allows yearn to step in if there is an exploit. Furthermore, assessing losses if stuck could mean that users don't actually get made whole because they exited with a loss, when we airdrop more funds to the strategy in case of an exploit.

[saltyfacu]

this is going to be turned into a Genlender plugin

[newmickymousse]

1. is this strategy clonable? I think we can deploy it in several vaults. If so, please add the clone function and make the name another init variable.
2. https://github.com/OpenOrg-gg/yearn-euler/blob/8d9654e3f3e63a864ed0a41c202379cc94a365a8/contracts/Strategy.sol#L218 you can write if(!emergencyMode) instead of comparing with == false.
3. https://github.com/OpenOrg-gg/yearn-euler/blob/8d9654e3f3e63a864ed0a41c202379cc94a365a8/contracts/Strategy.sol#L219-L220 you can store balanceOfWant() locally in a variable for the boolean expression and the for the deposit. Saves gas.
4. https://github.com/OpenOrg-gg/yearn-euler/blob/8d9654e3f3e63a864ed0a41c202379cc94a365a8/contracts/Strategy.sol#L258-L299 can all this logic be replace by withdrawSome(_amountNeeded)?
5. https://github.com/OpenOrg-gg/yearn-euler/blob/8d9654e3f3e63a864ed0a41c202379cc94a365a8/contracts/Strategy.sol#L306-L311 This logic can be replaced by eToken.withdraw(Math.min(balanceOfUnderlyingToWant(), availableBalanceOnEuler()))
6. I don't see how emergencyMode makes sense. We already have emergencyExit on strategies that you can use if you want to check whether this strategy is under an emergency situation.

[newmickymousse]

1. https://github.com/OpenOrg-gg/yearn-euler/blob/5ac14b2bb4905fd32d2cab9fde5b8538cdc2346c/contracts/Strategy.sol#L114-L129 All these actions should happen in the _initializeThis and the constructor should could _initializeThis. The only one that should not be on the initializeThis is the isOriginal variable setting. You can do that directly on Line 112.
2. https://github.com/OpenOrg-gg/yearn-euler/blob/5ac14b2bb4905fd32d2cab9fde5b8538cdc2346c/contracts/Strategy.sol#L279 This variable is not being used.
3. https://github.com/OpenOrg-gg/yearn-euler/blob/5ac14b2bb4905fd32d2cab9fde5b8538cdc2346c/contracts/Strategy.sol#L310 Why is this function public and not external? Is it intended to be used within the SC?
4. https://github.com/OpenOrg-gg/yearn-euler/blob/5ac14b2bb4905fd32d2cab9fde5b8538cdc2346c/contracts/Strategy.sol#L325 Here we are hardcoding the strategistSMS as treasury or who is going to keep the eul tokens. I think it's better to change this in the future and also eventually create a treasury for this exactly. @dudesahn do you share my opinion? or should be doing somehting else?
5. https://github.com/OpenOrg-gg/yearn-euler/blob/5ac14b2bb4905fd32d2cab9fde5b8538cdc2346c/contracts/Strategy.sol#L437-L443 These two methods can also be external if they are not intended to be used inside the contract.

[dudesahn]

If you declare something as an address, you don't need to bother re-casting it as an address. so on both L101 and L102 you can remove the address() portion and just leave the address itself

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L101

[dudesahn]

What is this address, as well as the one on L202? Would be good to have at least a comment explaining what they are. Also, similarly here, I don't think you need to cast these with address() and can just input them directly as args.

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L199

[dudesahn]

no need to set this to zero, will automatically be zero. can remove L206 completely and L100 can just be address public tradeFactory;

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L100

[dudesahn]

You already have a function for this, so estimatedTotalAssets() should just return balanceOfWant().add(balanceOfUnderlyingToWant()); and you can delete L217 https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L219

[dudesahn]

this var doesn't appear to be used

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L331

[dudesahn]

_withdrawSome() should just return the postBalance, as withdraw() on baseStrategy expects liquidatePosition() to return the total amount of assets freed

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L246

[dudesahn]

Probably good to add a comment here that calling this will result in losses on any funds that happen to be borrowed out that would then be realized later as profit if liquidity did return

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L342

[dudesahn]

This isn't used anywhere, can remove

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L271

[dudesahn]

This seems like a waste of gas, shouldn't estimatedTotalAssets() stay the same from where you called it above?

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L275

[dudesahn]

Just to confirm, when is this value updated in the contract? Is it only when we interact with the contract, does it update when anyone interacts with the contract, or does it just increase on its own?

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L232

[dudesahn]

I think I mentioned this before, but I'm still unclear why you are doing _loss plus what the real loss is. that _loss that you're adding to should already be zero, so there's no reason to add to it, just assign it the value.

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L277

[dudesahn]

does this line do anything? appears to just be a view, which would be useless here

https://github.com/OpenOrg-gg/yearn-euler/blob/f607abaf20955b678bcc63103cc25d0fd9e1ccbf/contracts/Strategy.sol#L295

[0xValJohn]

Replaced by https://github.com/yearn/yearn-strategies/issues/443