DAO Reviews

[Completed] Idle Finance aave v2 wrapper review Ask

Ask Reviews
#1
  1. Who you are and a brief description of the feature/project
    I’m William from https://idle.finance, yield aggregator and rebalancing protocol
  2. What’s the scope of the review? (e.g. github link, code snippet, private sharing)
    Our community would love to test the dao.reviews model by requesting a review for our new aave v2 wrapper idle-contracts/IdleAaveV2.sol at develop · Idle-Labs/idle-contracts · GitHub + the diff (here | here | here) from our old IdleCompound wrapper to our new IdleCompoundETH wrapper. The aave wrapper contract is used as a proxy to mint and redeem from Aave v2 (You can find the current running version for aave v1 here) while the IdleCompoundETH (which is a copy of the audited IdleCompound wrapper with around 5 lines changed) is used as a proxy to mint and redeem ETH in Compound (while we only support WETH). The code for this wrapper it’s already used in one of our beta strategy, more info here
  3. What kind of review do you need? (e.g. security, high level, gas optimization…)
    We need a security review
  4. What’s the deadline? (e.g. 2 weeks, a month)
    The sooner the better :slight_smile: The review should not take long btw ideally, given that’s around ~100 LOC
  5. Optional skills/level required for the reviewers
    Medium level, code for the 2 wrappers is self-contained
  6. Incentives/Rewards for reviewers
    30-60 IDLE per reviewer depending on the report and findings (max 2-3 reviewers for this review)
4 Likes
#2

Here’s my peer review:
https://docs.google.com/document/d/1bg9MW7z3qnct6BdqPHhwABOi1ZmTjywl5ZQwKXWqOyQ/edit?usp=sharing

I’d say the biggest thing is ensuring that onlyIdle is an address that can be trusted

This pattern seems to be present in other wrappers so it may be fine

Hope this helps!

2 Likes
#3

Link is now public, if you had issues accessing try again

#4

Hi @AlexTheEntreprenerd , thanks for posting this, these are my comments

  • Double Check Please → this is the intended behaviour. This contract never holds funds
    at the end of txs, mint and redeem are called only from the main idleToken contract (and should only be called by this contract).
    IdleToken contract sends funds to the wrapper, the wrapper forward them to the lending protocol
    and then it receives interest bearing assets (eg aDAI, cDAI, …) which are then sent back to the idleToken contract, all in the same tx.
  • "Initialized" is set but never used → it’s actually used to ensure that _approveProvider wont be called more than once
  • Addresses are set in the constructor → we are using solidity 0.5.16 so immutable is not available
  • Inconsistent return → this should return 0 if the minted amount is 0, which is the current behaviour, so not sure why it’s considered inconsistent
#5

Wouldn’t you want to return the amount also when it’s greater than 0?

#6

Makes sense

Makes sense, any specific reason why you are using 0.5.16 over more recent?

#7

It’s already implicitly returning aTokens given that it’s defined in the function signature (there are tests for this too).

All the repo and contracts are set to 0.5.16 (it’s a 1y and half old repo) so for now we have to keep this version.

#8

Makes sense
Also, I’ve seen you already went through an audit and implemented all fixes.

Would love to see a second review to see how they’d do it

2 Likes
#9

Taking a look to it…

#10

First review for the Aave2 connector

Going ahead with IdleCompoundETH upgrade

2 Likes
#11

First review for the CompoundETH connector

Looking forward @bugduino :handshake:

1 Like
#12

Thanks Emiliano, here my comments and fixes for aave v2 Review Aave V2 Connector · Issue #6 · Idle-Labs/idle-contracts · GitHub

1 Like
#13

Review commit for IdleCompoundETH here Review CompoundETH · Issue #7 · Idle-Labs/idle-contracts · GitHub

1 Like
#16

Checked the fixes and comments, LGTM, closed the issue :handshake:

1 Like
#17

Checked the fixes, LGTM, closed the issue :handshake:

Anyway I’ve appended an optional comment for consistency with the other fix you applied for Aave connector :wink:

1 Like
#18

Great, added comment + commit Review CompoundETH · Issue #7 · Idle-Labs/idle-contracts · GitHub

1 Like
#19

Great LGTM! :handshake:

1 Like
#20

I think that the review can be considered concluded now and we can proceede with the payment of the bounties.

  • @emilianobonassi 60 IDLE: the review was accurate and with lots of implementable fixes and improvements. This is the overall quality expected for reviews imo
  • @AlexTheEntreprenerd 10 IDLE: the review should have been definetely more detailed and it lacked of actionable insights and fixes, given that no comment was incorporated, so that’s why I would reward it below 30 IDLE, but I would still want to grant Alex something for the time spent on this

I think you guys can post an address here and then the Idle Treasury Committee will proceede with the bounties

1 Like
#21

You live and you learn I appreciate the tip:
0xF9B2819B90697BE4f5D7AEF7AD9Cffe1f65e3d29

1 Like
#22

Thanks @bugduino

Feel free to send the reward at 0x394495a3800d1504b5686d398836baefebd0c5b7

@AlexTheEntreprenerd, thanks for your help! If you want we can discuss off-line via tg how to improve your next reviews, feel free to ping me :wink:

1 Like