SmartTreasuryBootstrap and FeeCollector rev 2

[bugduino]

I made a new review after changes with @gravityblast this are our new comments:

SmartTreasuryBootstrap:

You should be able to remove this line https://github.com/AsafSilman/idle-smart-treasury/blob/main/contracts/SmartTreasuryBootstrap.sol#L256 safely because it's already checked in the constructor and cannot be changed.

To make this more resilient in case of emergency
https://github.com/AsafSilman/idle-smart-treasury/blob/957ec663ba1ea51096120cdab67428c765d764eb/contracts/SmartTreasuryBootstrap.sol#L279-L284
we should also add a way to recover funds in case any of the previous methods (swap, initialize or bootstrap) fail for any reason so when renounced has not been set. So we propose to remove the onlyOwner modifier and replace the require statement with this one require((msg.sender == owner() && renounced == true) || msg.sender == timelock, "Only admin");.

FeeCollector:
Generally speaking we should consider that IDLE and WETH can be present in this contract at anytime (maybe for future fees)
so something may needs to be slightly changed.

This https://github.com/AsafSilman/idle-smart-treasury/blob/main/contracts/FeeCollector.sol#L146 should be set to 1 as in the SmartTreasuryBootstrap.

This comment https://github.com/AsafSilman/idle-smart-treasury/blob/957ec663ba1ea51096120cdab67428c765d764eb/contracts/FeeCollector.sol#L154 should probably be something like
// deposit all swapped WETH into balancer pool + the already present weth balance.

Here https://github.com/AsafSilman/idle-smart-treasury/blob/957ec663ba1ea51096120cdab67428c765d764eb/contracts/FeeCollector.sol#L168-L169 you should check that feeBalances[0] > 0 to be sure to deposit something.

When we set a new beneficiary here https://github.com/AsafSilman/idle-smart-treasury/blob/957ec663ba1ea51096120cdab67428c765d764eb/contracts/FeeCollector.sol#L213 the old allocations are not respected. A deposit() call should be made before and the setSplitAllocation should get a bool param to skip the deposit when called from here. The same problem is true for also for removeBeneficiaryAt and replaceBeneficiaryAt.

Here
https://github.com/AsafSilman/idle-smart-treasury/blob/957ec663ba1ea51096120cdab67428c765d764eb/contracts/FeeCollector.sol#L376-L391
you are potentially withdrawing more IDLE and WETH if those are already present in the contract so before the exitPool you should loop on treasuryTokens and save IDLE and WETH balances, then do exitPool and after that loop again on treasuryTokens and calc diff generated by exiting and transfer those tokens directly.

As a side note would be great to have uint256 instead of uint across all contracts and some of the unused comments removed (like this one or this)

[bugduino]

I think we should also directly add another whitelisted wallet for the deposit method so it can be managed via an off chain bot. You can use Idle rebalancer address potentially but we should probably discuss this in the forum too

[asafsilman]

@bugduino What is the address for the rebalancer? I will add it as a white-listed address in the migration script

[bugduino]

Will post it in the forum so it's 'recorded' there

[asafsilman]

I've address the points made for the
SmartTreasuryBootstrap:

• I've updated withdraw to use that code snippet

FeeCollector:

• removed old comments
• add balance check on deposit()
• addBeneficiaryAddress(), removeBeneficiaryAt(), replaceBeneficiaryAt () makes a to call to setAllocation(), which now properly respects the previous allocation by calling deposit() before updating to the new allocation. I've updated deposit() to iterate over allocations as apposed to beneficiaries to ensure the correct allocation is honoured
  https://github.com/AsafSilman/idle-smart-treasury/blob/6e9f4f2163860e636478a39d84ad499675e9fb92/contracts/FeeCollector.sol#L164-L169
• withdrawUnderlying correctly calculates pool amounts, excluding other amounts that may be present
  https://github.com/AsafSilman/idle-smart-treasury/blob/6e9f4f2163860e636478a39d84ad499675e9fb92/contracts/FeeCollector.sol#L390-L414
• all units should now be uint256

I've also added the rebalancer to the feeCollector configuration migration as discussed https://gov.idle.finance/t/update-smart-treasury-propsal-code-review/179/23

Closing this ticket for now :)

[gravityblast]

great work @asafsilman !
I'll add some comments here in this issue.

When we update the beneficiaries with the new code, I think it works correctly for addBeneficiaryAddress, but not for removeBeneficiaryAt and replaceBeneficiaryAt.
In those cases we set the new allocations at the end, but before calling setSplitAllocation (that calls deposit) we already update the beneficiaries, so we loop already on the new array.
I guess the fix can be to call setSplitAllocation before updating the beneficiaries array:

This one should work already, but it would be better to have all of them in the same way:
https://github.com/AsafSilman/idle-smart-treasury/blob/b37dc58aef0bec309caa42b26f1dc7eb60f872d0/contracts/FeeCollector.sol#L215-L221

Here we could move the call from line 251 to 246:
https://github.com/AsafSilman/idle-smart-treasury/blob/b37dc58aef0bec309caa42b26f1dc7eb60f872d0/contracts/FeeCollector.sol#L243-L251

And here from 269 to 266:
https://github.com/AsafSilman/idle-smart-treasury/blob/b37dc58aef0bec309caa42b26f1dc7eb60f872d0/contracts/FeeCollector.sol#L263-L270

[gravityblast]

unless we can set a more precise number, I would set the amountOutMin to at least 1 instead of 0:

https://github.com/AsafSilman/idle-smart-treasury/blob/b37dc58aef0bec309caa42b26f1dc7eb60f872d0/contracts/FeeCollector.sol#L141-L150

[gravityblast]

In withdrawUnderlying we are checking now the pre and post balances of the pool.
I would change it to check the balances of the contract itself (FeeCollector) so that in case there are fees or other things added/removed during the exitPool call, we keep the right calculations:
https://github.com/AsafSilman/idle-smart-treasury/blob/b37dc58aef0bec309caa42b26f1dc7eb60f872d0/contracts/FeeCollector.sol#L391-L410

[asafsilman]

Changes added in @ 9feed2c

[asafsilman]

Please close this ticket after your review :)

[gravityblast]

I would set the amountOutMin to at least 1 instead of 0:

that's done, thank you!

I would change it to check the balances of the contract itself (FeeCollector)

cool as well! 👍

I guess the fix can be to call setSplitAllocation before updating the beneficiaries array:

For this part I would avoid having a new function with almost the same code.
We could add a param in _setSplitAllocations, something like bool executeDeposit.
So that if it's called externally, we would call it with true.
Internally, every time we change the beneficiaries we could:

• deposit
• change beneficiaries array
• call setSplitAllocations with executeDeposit = false

What do you think @asafsilman

[asafsilman]

I think that _setSplitAllocations should remain an internal function, and setSplitAllocations will be public-facing (only accessible by admin). But I will change the logic slightly for setSplitAllocations to call on _setSplitAllocations, this way there is no repeated code.

This way the public method will work as intended, and the internal method can be used in the case of replaceBeneficiaryAt and for the public setSplitAllocations.

If I added a boolean to the public function is exposes the risk for the incorrect fee weights to be honoured.

[asafsilman]

added in 730847a

[asafsilman]

Removed convoluted code in d2b3064