TrueFi Lender

[vany365]

Description:

Commit (hash):

Tag:

Repo: https://github.com/tapired/TrueFiLenderStrat/blob/master/contracts/Strategy.sol

Scope: (List link to files)

• File.sol

Due Diligence: https://hackmd.io/u0podgTyTLuTbx8IZ0f6iQ

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

Deployed Contract https://etherscan.io/address/0xe683acdd3d2611745c6636497f106b65750a6c6f

Review Ongoing By:

• [x] @charlesndalton
• [x] @tonkers-kuma

Review Completed By:

• [x] @charlesndalton
• [ ] @tonkers-kuma

[decaf-addict]

suggestion: Add in the token name as well to differentiate between clones https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L60

i.e. https://github.com/tonkers-kuma/strategy-ssb/blob/7dcefbb093c3d740836b30fe9f55ef12e1ef8d5d/contracts/Strategy.sol#L164

[decaf-addict]

I think this method would be much more useful if you specify the amount of lps: https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L106

[decaf-addict]

redundant since you already have a setter to do this externally https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L44

[decaf-addict]

2nd pass seems fine. I think you may need more defense around exiting your position. From what I read, TrueFi takes a pretty savage withdrawal fee and scales up the fee depending on utilization rate, this would be especially bad during market crunch when everyone's trying to exit. You may want to add some logic around reverting if the loss becomes too high to prevent accidental exits that would violate our "only up" soft rule.

[decaf-addict]

Add external onlyVaultManager versions of these methods https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L169 https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L177

[decaf-addict]

set to 0, otherwise, this could mean a really large value for tokens with small decimals https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L279

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L33

Why not just declare tru as IERC20?

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L117-L128

Suggestion: instead of having this return a boolean and then wrapping the response with require(), have it return nothing and just call _checkPath

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L58-L61

Suggestion: include the asset symbol, as done here:

https://github.com/tonkers-kuma/strategy-bancorV3/blob/a55c273b81bc36505881a41b8dc104ba6ef2aed7/contracts/Strategy.sol#L80-L88

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L68

Does 1e18 work for wants that don't have 18 decimals, such as USDC?

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L71

Why are some of these internal and some of these public?

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L75

Nit: don't need the underscore in front for public function

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L143

Suggestion: 'eta' -> 'assets'

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L152-L154

I believe you can declare variables inside the tuple deconstructor i.e., (uint256 _amountFreed, ...

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L192

Why call again?

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L190-L195

This function needs to take _debtOutstanding into account, such as in the below:

https://github.com/charlesndalton/generic-tokemak-strat/blob/7b12380135915b28a120fdab4aeff7edd481dbed/contracts/Strategy.sol#L186-L190

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L192-L199

Good that these are separate :)

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L239

Not required, as this only gets triggered from BaseStrategy when emergencyExit is true

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L257-L260

What about transferring the LP tokens? I'm a little surprised tests didn't catch this

[tapired]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L68

Does 1e18 work for wants that don't have 18 decimals, such as USDC?

In the getVirtualPrice() function we are multiplying with 10**18 so that division is for that to cancel

[tapired]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L71

Why are some of these internal and some of these public?

Internal ones are for the strategy handling the virtual price doesnt necessarily needed from public view. I thought maybe lots of balance getter functions would look like mess

[tapired]

I think this method would be much more useful if you specify the amount of lps:

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L106

I was thinking it would be more related with the strategy. You can always queue any amount in the pool contract itself , so instead of specifying an amount we are auto querrying the strategies current exit fee

[tapired]

https://github.com/tapired/TrueFiLenderStrat/blob/6443ba2dcb01897f091b1cef80d040add73b87c3/contracts/Strategy.sol#L257-L260

What about transferring the LP tokens? I'm a little surprised tests didn't catch this

gauge.exit() claims and unstakes all rewards pool.liquidExit() converts all LP's to want
so the strategy doesnt have any LP. Reason why I also thought exiting the pool (getting back to want from LP) would be better is that most of the want tokens the strategy handles are in form of LP. So ideally , we should always have LP at maximum and when we migrate we basically migrate very small portion of the funds and rest will go to governance sweeping. Maybe I am wrong with this approach. How are those things handled in situations where coming back to want token from the yield token has 2 steps? (want ==> LP ==> stake LP )

[charlesndalton]

https://github.com/tapired/TrueFiLenderStrat/blob/2ef494ba315bf3d27214bd55591f629cd591f758/contracts/Strategy.sol#L134-L138

I like these checks

[charlesndalton]

I see a few tests failing in CI, but @tapired mentioned that these are passing locally. Maybe a brownie issue. Other than that, LGTM!

[saltyfacu]

it's in ape.tax: https://etherscan.io/address/0xe683acdd3d2611745c6636497f106b65750a6c6f

[decaf-addict]

clean up all swap related code now that yswaps is being used (swap paths, unirouter, swap fn, constructor param, etc) i.e. https://github.com/tapired/TrueFiLenderStrat/blob/2ef494ba315bf3d27214bd55591f629cd591f758/contracts/Strategy.sol#L125

[decaf-addict]

arguably not necessary. TF is only necessary to realize selling profit, but shouldn't be required to enter a position https://github.com/tapired/TrueFiLenderStrat/blob/2ef494ba315bf3d27214bd55591f629cd591f758/contracts/Strategy.sol#L150

[decaf-addict]

I would maybe put this inside prepareReturn or tendTrigger so that we could put harvest on keeper and skip the need for a manual claim, unless this strat is meant to be manual https://github.com/tapired/TrueFiLenderStrat/blob/2ef494ba315bf3d27214bd55591f629cd591f758/contracts/Strategy.sol#L178

[decaf-addict]

Quoting Dudesahn here, Add this to harvestTrigger for keeper

Spamming all mainnet strategies in review—please please consider checking ethereum baseFee prices for harvestTrigger similar to what I do here; and just in general planning around automating as much as you can with harvestTrigger:

// check if the current baseFee is below our external target
function isBaseFeeAcceptable() internal view returns (bool) {
    return
        IBaseFee(0xb5e1CAcB567d98faaDB60a1fD4820720141f064F)
            .isCurrentBaseFeeAcceptable();
}

Happy to discuss if you have any questions 🙂