Sturdy Gen Lender Strategy Review

[newmickymousse]

Claiming a strategy Protocol name: Sturdy

Brief Strategy Description: Deposits stablecoins (USDC, USDT, or DAI) to Sturdy's lending pool

Due Diligence Review Protocol Due Diligence and Path to Prod: here

Once a strategy's due diligence has been approved by the team, the strategist can begin codifying it for code review Strategy Review Description: Deposits stablecoins (USDC, USDT, or DAI) to Sturdy's lending pool. Sturdy is a two-sided money market, whose primary differentiator is that collateral is staked, with the yields from staking being used to improve rates for lenders. Yields are paid out automatically in-kind, requiring no additional logic to harvest. There are no fees to deposit or withdraw from Sturdy.

Commit (hash): c1b702198249abc12aacc766e66870ec8f278e92

Tag:

Scope: contracts/GenericSturdy.sol

Additional References or Repo (Link to underlying protocols or relevant documents): docs.sturdy.finance

Deployed Contract (etherscan link):

Review Ongoing By:

• [x] @newmickymousse
• [x] @Schlagonia

Review Completed By:

• [x] @newmickymousse
• [x] @Schlagonia

[newmickymousse]

1. https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/c1b702198249abc12aacc766e66870ec8f278e92/contracts/GenericLender/GenericSturdy.sol#L221-L223 can you store this address/IlendingPool into a variable to avoid calling it to frequently? I have checked the contracts and seems to be a constant (correct me if I'm wrong). You can assign the variable in the initialize function.
2. https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/c1b702198249abc12aacc766e66870ec8f278e92/contracts/GenericLender/GenericSturdy.sol#L39 WETH not being used in the contract
3. What are you using isIncentivised for? Do you have incentivized lending pools? If so, how do you harvest them? Is it different from a non-incentivized pool harvest?

[sforman2000]

Thanks @newmickymousse, these have been fixed in 8aaf6f9972efd1269bdc5d300a4871ab8b69e838

3. What are you using isIncentivised for? Do you have incentivized lending pools? If so, how do you harvest them? Is it different from a non-incentivized pool harvest?

We don't have incentivized lending pools yet but plan to at some point in the future. We've removed this for now since we don't know how they'll be implemented and we don't plan on launching them in the near-term.

[Schlagonia]

I would remove the Atoken variable from the constructor and cloning functions. Will make cloning much easier and you can just set it using the lendingPool in the _initiliaze. May want to add a check the returned aToken != address(0)

[Schlagonia]

Should update this and this modifier to use .strategist() instead of management(). Will need to add that function to the IBaseStrategy interface as well

[Schlagonia]

Would probably be good to check want balance here as well https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/master/contracts/GenericLender/GenericSturdy.sol#L128

You can also check against "dust" instead of 0. You probably know better than us how you protocol handles super small amounts and if its possible leave behind or withdraw tiny amounts.

[Schlagonia]

These two lines make a call that is just made in the aprAfterDeposit() before this function. While aprAfterDeposit() is a view function it is used in every harvest so limiting gas costs is helpful. Can you send the the neccesary variables as function params to avoid redundant calls for large dataTypes and memory usage

[Schlagonia]

It appears that the current value for reserveData.currentLiquidityRate of all the stable coins is currently 0? Am I reading this correct? I would assume its cause no one is paying interest to borrow?

If so is this expected evert to change? If that is always the expected behavior is there any point in even calcluating this

[Schlagonia]

Can just declare the variable in this line in the function as the return variable and delete this line and L235

[Schlagonia]

Do you know how confident you are in this calculation being accurate?

Are you able to use just a linear difference cause you really are just dividing up staked rewards evenly over all deposits rather than an interest rate model?

[sforman2000]

Thanks @Schlagonia, we'll get to work on these.

It appears that the current value for reserveData.currentLiquidityRate of all the stable coins is currently 0? Am I reading this correct? I would assume its cause no one is paying interest to borrow?

If so is this expected evert to change? If that is always the expected behavior is there any point in even calcluating this

Borrowers start paying interest if the utilization on the borrowed asset goes above 90%, so we still need to calculate it.

Do you know how confident you are in this calculation being accurate?

Are you able to use just a linear difference cause you really are just dividing up staked rewards evenly over all deposits rather than an interest rate model?

Yep, that's exactly what we're doing, and it's been very accurate empirically. The only exception is when utilization goes above 90% as mentioned above, in which case lenders earn interest on top of the collateral staking yield.

[sforman2000]

@Schlagonia We've pushed fixes in 739ce4be6cd8eb7117707d840203f17442d7e5ac

[Schlagonia]

You will still need to add the strategist call the the IBaseStrategy interface https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/master/contracts/GenericLender/GenericLenderBase.sol

Its just a view funtion that returns an address

[sforman2000]

@Schlagonia fixed in b317e542afa63c2d07b9b3a9f401ccaee4a231d2

[Schlagonia]

Couple of small gas things:

no need to save this line to its own variable,

this for loop calls reserves[i] 3 times. Probably cheaper to declare an address reserve; outside the loop and set it during each loop to only read from the array once

[Schlagonia]

you can also remove all the instances of keep3r. Including state variable, setter function and modifier since this doesnt get harvested

[sforman2000]

@Schlagonia fixed in ffc8deb814beaf80118dc29a231de902ed03b69e

[Schlagonia]

@Schlagonia fixed in ffc8deb

Sorry, meant you can get rid of the whole keeper modifier

[sforman2000]

Sorry, meant you can get rid of the whole keeper modifier

Understood, removed it in af4b126103b9c047bf5bea00f96d9fda0430f3ef

[newmickymousse]

LGTM

[Schlagonia]

LGTM

[Schlagonia]

Deployed Ape Tax version https://etherscan.io/address/0x681f12990dc1c186c0de351406d1e9aca625352b#code

[sforman2000]

@Schlagonia @newmickymousse We updated the APR calculation in d0ad9d1b0fc9bc9025f1fad0988ae1f1aea0d841 to fix the issue where the gas cost was too high during harvest.

[newmickymousse]

new changes LGTM

[Schlagonia]

LGTM as well

[newmickymousse]

new ape.tax deployed contract: https://etherscan.io/address/0xB865AAf1f9f60630934739595f183C4900f65ed9

[Schlagonia]

new ape.tax deployed contract: https://etherscan.io/address/0xB865AAf1f9f60630934739595f183C4900f65ed9

This is the update one directly to the plugin https://etherscan.io/address/0xe86a478b607804ebc3fcc4b7d1b8730dd74cf095

[storming0x]

Comment:

This version of the GenLenderBase has the old strategist role as management which latest version has replaced w management role since this is our default SMS Multisig for our current strategies

https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/ca77cf47cd32773976bf340ce48a68b93543c099/contracts/GenericLender/GenericLenderBase.sol#L93

https://github.com/flashfish0x/yearnV2-generic-lender-strat/blob/f55c63ef3feaa03fb61e262ed4954db3e7f1cddc/contracts/GenericLender/GenericLenderBase.sol#L93

[iris112]

Hi @storming0x I have just replaced the role. https://github.com/sturdyfi/yearnV2-generic-lender-strat/commit/8b59c11f3a774392649e48ce3c2f60d4982abe08

Regards.

[storming0x]

contracts/GenericSturdy.sol

Hey @iris112 i think the change should be vault.management() since the basestrategy interface doesnt have that method call from the repo yearn vaults.. see example

https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/BaseStrategy.sol#L360

https://github.com/sturdyfi/yearnV2-generic-lender-strat/commit/8b59c11f3a774392649e48ce3c2f60d4982abe08

Sorry for the confusion i think the link i sent has a diff implementation of base strategy, so using vault.management() is a safer bet.

FYI im still reviewing the code so may come up with more things and comment here.

[storming0x]

Critical?

Although the initialize() method here

https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/8b59c11f3a774392649e48ce3c2f60d4982abe08/contracts/GenericLender/GenericSturdy.sol#L46

https://github.com/sturdyfi/yearnV2-generic-lender-strat/blob/8b59c11f3a774392649e48ce3c2f60d4982abe08/contracts/GenericLender/GenericSturdy.sol#L122

seems to repeat the same code, its potentially dangerous to leave this code without a lock once initialized, since it can be call over and over from any external actor and it shouldnt be the case for initialized calls to be available more than once.

Also if there are new versions of this, may add code that only needs to run once and forget that a lock guaranteeing only once execution is not in place, just another thing to juggle mentally which i think can be easily fix with a lock.

i suggest we use some time of lock to be sure the code in initialize is only executed once

[iris112]

Hi @storming0x I have just fixed the above issues.

https://github.com/sturdyfi/yearnV2-generic-lender-strat/commit/1a4e108b30d6f2f1f26fc94de46f3287f457c6f2

Regards, Thanks.

[Schlagonia]

Deployed Version with the most recent changes https://etherscan.io/address/0xaf363c4b4104c88ff7578b1d9898eaa1bdc0d257#code