- Who you are and a brief description of the feature/project
I’m William from https://idle.finance, yield aggregator and rebalancing protocol
- 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
- What kind of review do you need? (e.g. security, high level, gas optimization…)
We need a security review
- What’s the deadline? (e.g. 2 weeks, a month)
The sooner the better The review should not take long btw ideally, given that’s around ~100 LOC
- Optional skills/level required for the reviewers
Medium level, code for the 2 wrappers is self-contained
- Incentives/Rewards for reviewers
30-60 IDLE per reviewer depending on the report and findings (max 2-3 reviewers for this review)
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!
Link is now public, if you had issues accessing try again
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
_approveProviderwont be called more than once
Addresses are set in the constructor→ we are using solidity 0.5.16 so
immutableis 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
Wouldn’t you want to return the amount also when it’s greater than 0?
Makes sense, any specific reason why you are using 0.5.16 over more recent?
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.
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
Taking a look to it…
First review for the Aave2 connector
Going ahead with IdleCompoundETH upgrade
First review for the CompoundETH connector
Looking forward @bugduino
Thanks Emiliano, here my comments and fixes for aave v2 Review Aave V2 Connector · Issue #6 · Idle-Labs/idle-contracts · GitHub
Review commit for IdleCompoundETH here Review CompoundETH · Issue #7 · Idle-Labs/idle-contracts · GitHub
Checked the fixes and comments, LGTM, closed the issue
Checked the fixes, LGTM, closed the issue
Anyway I’ve appended an optional comment for consistency with the other fix you applied for Aave connector
Great, added comment + commit Review CompoundETH · Issue #7 · Idle-Labs/idle-contracts · GitHub
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
You live and you learn I appreciate the tip: