Kasper initial reviews #16

haythemsellami · 2024-06-03T15:21:48Z

No description provided.

haythemsellami · 2024-06-03T15:22:07Z

src/BalanceForwarder.sol

@@ -51,7 +51,7 @@ abstract contract BalanceForwarder is IBalanceForwarder {
        isBalanceForwarderEnabled[_sender] = true;
        IBalanceTracker(balanceTracker).balanceTrackerHook(_sender, _senderBalance, false);

-        emit EnableBalanceForwarder(_sender);
+        emit EnableBalanceForwarder(_sender); // @review: do we want to emit the event even if no state change?


Yeah will update this.

haythemsellami · 2024-06-03T15:22:18Z

src/BalanceForwarder.sol

@@ -63,6 +63,6 @@ abstract contract BalanceForwarder is IBalanceForwarder {
        isBalanceForwarderEnabled[_sender] = false;
        IBalanceTracker(balanceTracker).balanceTrackerHook(_sender, 0, false);

-        emit DisableBalanceForwarder(_sender);
+        emit DisableBalanceForwarder(_sender); // @review: do we want to emit the event even if no state change?


Same here, will update.

haythemsellami · 2024-06-03T15:23:06Z

src/FourSixTwoSixAgg.sol

@@ -47,7 +47,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
    ESRSlot internal esrSlot;

    /// @dev Total amount of _asset deposited into FourSixTwoSixAgg contract
-    uint256 public totalAssetsDeposited;
+    uint256 public totalAssetsDeposited; // @review: this should rather be internal not to be misleading


I would keep it public, no harm in providing more data to the off-chain world, also may help if someone want to know how much interest is smeared to the deposit.

haythemsellami · 2024-06-03T15:24:18Z

src/FourSixTwoSixAgg.sol

+        // @review: why don't we simplify the inteface here? the cash strategy can be passed in the arrays, no need for additional
+        // _initialCashAllocationPoints parameter and special handling
+
        strategies[address(0)] =


It is better to be explicit I think in this case, and make it clear that this is a separate cash reserve than the rest of the strategies array...

haythemsellami · 2024-06-03T15:25:13Z

src/FourSixTwoSixAgg.sol

@@ -125,6 +128,10 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
            strategies[_initialStrategies[i]] =
                Strategy({allocated: 0, allocationPoints: uint120(_initialStrategiesAllocationPoints[i]), active: true});

+            // @review: shall we check that the strategy is an ERC4626 vault same as we do in addStrategy?
+            // @review: shouldn't we add the strategy to the withdrawal queue?


Yes withdrawalQueue here is missing.

haythemsellami · 2024-06-03T15:26:25Z

src/FourSixTwoSixAgg.sol

@@ -125,6 +128,10 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
            strategies[_initialStrategies[i]] =
                Strategy({allocated: 0, allocationPoints: uint120(_initialStrategiesAllocationPoints[i]), active: true});

+            // @review: shall we check that the strategy is an ERC4626 vault same as we do in addStrategy?
+            // @review: shouldn't we add the strategy to the withdrawal queue?
+            // @review: we should prevent strategy duplicates, otherwise the accounting might get messed up


Is there a diff between having a strategy with 1 allocation config vs multiples?

I think so. you will write to strategies multiple times if there are duplicates, overriding the previously set setting. at the same time, you'll be increasing totalAllocationPoints.

haythemsellami · 2024-06-03T15:26:41Z

src/FourSixTwoSixAgg.sol

@@ -166,6 +173,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
        for (uint256 i; i < _strategies.length; ++i) {
            _rebalance(_strategies[i]);
        }
+        // @review: wouldn't it be better to gulp just once here rather than on every rebalance?


This is updated already.

haythemsellami · 2024-06-03T15:29:56Z

src/FourSixTwoSixAgg.sol

@@ -426,10 +451,11 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
            IERC4626 strategy = IERC4626(withdrawalQueue[i]);

            _harvest(address(strategy));
-            _gulp();
+            _gulp(); // @review: wouldn't it be more efficient to gulp just once after the loop?


This is updated.

haythemsellami · 2024-06-04T12:49:36Z

src/FourSixTwoSixAgg.sol

@@ -107,15 +107,18 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
        address _asset,
        string memory _name,
        string memory _symbol,
-        uint256 _initialCashAllocationPoints,
+        uint256 _initialCashAllocationPoints, // @review: should be uint120 or checked if it's <= type(uint120).max


haythemsellami · 2024-06-04T12:51:07Z

src/FourSixTwoSixAgg.sol

        address[] memory _initialStrategies,
-        uint256[] memory _initialStrategiesAllocationPoints
+        uint256[] memory _initialStrategiesAllocationPoints // @review: should be array of uint120 or checked if each element is <= type(uint120).max


haythemsellami · 2024-06-04T13:28:59Z

src/FourSixTwoSixAgg.sol

@@ -251,6 +259,9 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
    /// @dev Can only be called by an address that have the STRATEGY_REMOVER_ROLE
    /// @param strategy Address of the strategy
    function removeStrategy(address strategy) external nonReentrant onlyRole(STRATEGY_REMOVER_ROLE) {
+        // @review: protect against mistakenly removing cash strategy. if address(0) passed, withdrawalQueue will get popped anyways.


haythemsellami · 2024-06-04T13:46:13Z

src/FourSixTwoSixAgg.sol

+        // @review: why don't check if the contract has enough assets to withdraw at this point?
+        // it would be at least one SLOAD less if no need to enter the loop
+


refactored the whole _withdraw()

haythemsellami · 2024-06-04T13:46:20Z

src/FourSixTwoSixAgg.sol

@@ -426,10 +451,11 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
            IERC4626 strategy = IERC4626(withdrawalQueue[i]);

            _harvest(address(strategy));
-            _gulp();
+            _gulp(); // @review: wouldn't it be more efficient to gulp just once after the loop?


updated already.

haythemsellami · 2024-06-04T13:48:07Z

src/FourSixTwoSixAgg.sol


            Strategy storage strategyStorage = strategies[address(strategy)];

+            // @review: seems like a maxWithdraw might be better suited here


haythemsellami · 2024-06-04T13:50:04Z

src/FourSixTwoSixAgg.sol

@@ -438,7 +464,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn

            // Update allocated assets
            strategyStorage.allocated -= uint120(withdrawAmount);
-            totalAllocated -= withdrawAmount;
+            totalAllocated -= withdrawAmount; // @review: why not write to storage once after the loop?


Will revisit this later, need to see if there is any weird impact on negative yield accounting when harvesting.

haythemsellami · 2024-06-04T13:56:40Z

src/FourSixTwoSixAgg.sol

@@ -482,7 +508,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn

        // Harvest profits, also gulps and updates interest
        _harvest(_strategy);
-        _gulp();
+        _gulp(); // @review: if _rebalance called in the loop, we'll gulp multiple times


haythemsellami · 2024-06-04T13:58:15Z

src/FourSixTwoSixAgg.sol

@@ -553,6 +580,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
            // TODO possible performance fee
        } else {
            // TODO handle losses
+            // @review: maybe better to do nothing than revert? not sure


I implemented now a better handling for negative yield

haythemsellami · 2024-06-04T14:01:26Z

src/FourSixTwoSixAgg.sol

+        // @review: will shares of this aggregator be used as collateral?
+        // if so, it might be worth to handle it the same way as we do in the EVK and forfeit recent rewards if in liquidation:
+        // https://github.com/euler-xyz/euler-vault-kit/blob/master/src/EVault/shared/BalanceUtils.sol#L57-L63


I don't think we are planning to use it as collateral.

haythemsellami · 2024-06-04T16:15:30Z

src/FourSixTwoSixAgg.sol

+        // @review: it seems that OZ allows self-transfers. if so, add check for to != from.
+        // if they're equal, the hook was already called above


Then I think we should just ignore hook's call for self-transfers

it's always good to update the tracker frequently for the benefit of everyone earning rewards, even if there was no actual value transfer

haythemsellami · 2024-06-04T16:19:47Z

src/FourSixTwoSixAgg.sol

+        // @review: currently we're using _msgSender() also for access control. do we want the EVC operators and sub-accounts to
+        // to be used for access control? if not, we need to override _checkRole and apply additional checks i.e. as it's done here:
+        // https://github.com/euler-xyz/euler-vault-kit/blob/master/src/EVault/shared/EVCClient.sol#L62-L78


I think it should be fine, also better to keep any EVC stuff abstracted away from here and have it as generic as possible.

note though that if it's not done, it might be very hard to verify the roles. this is because someone can delegate the control to i.e. operator via the EVC which is not easy to spot when analyzing. this is the reason why we decided to lock it down in the EVK, it's safer

haythemsellami · 2024-06-04T16:19:55Z

src/FourSixTwoSixAgg.sol

@@ -576,7 +609,7 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
    /// @return uint256 accrued interest
    function interestAccruedFromCache(ESRSlot memory esrSlotCache) internal view returns (uint256) {
        // If distribution ended, full amount is accrued
-        if (block.timestamp > esrSlotCache.interestSmearEnd) {
+        if (block.timestamp > esrSlotCache.interestSmearEnd) { // @review: wouldn't it be better with >= ?


haythemsellami · 2024-06-04T16:20:33Z

src/FourSixTwoSixAgg.sol

+    // @review: do we want to override _convertToShares and _convertToAssets to maintain 
+    // the same level of exchange rate manipulation protection as for other products?
+    // see: https://github.com/euler-xyz/euler-vault-kit/blob/master/src/Synths/EulerSavingsRate.sol#L177-L183


Is the default OZ implementation not enough?

if it won't be used as collateral (and you already said it won't), it should be fine

haythemsellami · 2024-06-04T19:02:42Z

Made fixes in this PR #17

review

4f85097

haythemsellami commented Jun 3, 2024

View reviewed changes

haythemsellami commented Jun 4, 2024

View reviewed changes

haythemsellami closed this Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kasper initial reviews #16

Kasper initial reviews #16

haythemsellami commented Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

kasperpawlowski Jun 6, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 3, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

kasperpawlowski Jun 6, 2024 •

edited

Loading

haythemsellami Jun 4, 2024 •

edited

Loading

kasperpawlowski Jun 6, 2024

haythemsellami Jun 4, 2024

haythemsellami Jun 4, 2024

kasperpawlowski Jun 6, 2024

haythemsellami commented Jun 4, 2024

		// @review: why don't check if the contract has enough assets to withdraw at this point?
		// it would be at least one SLOAD less if no need to enter the loop


		Strategy storage strategyStorage = strategies[address(strategy)];

		// @review: seems like a maxWithdraw might be better suited here

		// @review: it seems that OZ allows self-transfers. if so, add check for to != from.
		// if they're equal, the hook was already called above

Kasper initial reviews #16

Kasper initial reviews #16

Conversation

haythemsellami commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasperpawlowski Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

haythemsellami Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haythemsellami commented Jun 4, 2024

kasperpawlowski Jun 6, 2024 •

edited

Loading

haythemsellami Jun 4, 2024 •

edited

Loading