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): c1b7021

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:

• @newmickymousse
• @Schlagonia

Review Completed By:

• @newmickymousse
• @Schlagonia

[newmickymousse]

1. yearnV2-generic-lender-strat/contracts/GenericLender/GenericSturdy.sol

   Lines 221 to 223 in c1b7021

   	function _lendingPool() internal view returns (ILendingPool lendingPool) {
   	lendingPool = ILendingPool(protocolDataProvider.ADDRESSES_PROVIDER().getLendingPool());
   	}

   
   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. yearnV2-generic-lender-strat/contracts/GenericLender/GenericSturdy.sol

   Line 39 in c1b7021

   address public constant WETH = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);

   
   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 8aaf6f9

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 739ce4b

[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 b317e54

[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 ffc8deb

[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 af4b126

[newmickymousse]

LGTM

[Schlagonia]

LGTM

[Schlagonia]

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

[sforman2000]

@Schlagonia @newmickymousse We updated the APR calculation in d0ad9d1 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

yearnV2-generic-lender-strat/contracts/GenericLender/GenericLenderBase.sol

Line 93 in ca77cf4

modifier management() {

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

[iris112]

Hi @storming0x
I have just replaced the role.
8b59c11

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

8b59c11

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

yearnV2-generic-lender-strat/contracts/GenericLender/GenericSturdy.sol

Line 46 in 8b59c11

function initialize() external {

yearnV2-generic-lender-strat/contracts/GenericLender/GenericSturdy.sol

Line 122 in 8b59c11

function _initialize() internal {

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.

1a4e108

Regards,
Thanks.

[Schlagonia]

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