feat: add erc4626 dummy delayed strategy by 0xKoaj · Pull Request #89 · Balmy-protocol/earn-periphery

0xKoaj · 2024-10-22T15:45:21Z

No description provided.

linear · 2024-10-22T15:45:25Z

BLY-3229 Build "dummy" strategy that forces delayed withdrawal, to test everything

github-actions · 2024-10-22T15:51:59Z

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

missing-zero-check (2 results) (Low)
calls-loop (3 results) (Low)
timestamp (3 results) (Low)
naming-convention (1 results) (Informational)

missing-zero-check

Impact: Low
Confidence: Medium

ID-0
ERC4626DelayedStrategy.constructor(IGlobalEarnRegistry,IEarnVault,address,string,IDelayedWithdrawalAdapter).farmToken_ lacks a zero-check on :
- farmToken = farmToken

earn-periphery/src/strategies/instances/erc4626/ERC4626DelayedStrategy.sol

Line 36 in 5342425

address farmToken_,

ID-1
ERC4626DelayedWithdrawalAdapter.constructor(IGlobalEarnRegistry,address,uint256).farmToken_ lacks a zero-check on :
- farmToken = farmToken

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Line 33 in 5342425

    
           constructor(IGlobalEarnRegistry _registry, address farmToken_, uint256 delay_) {

calls-loop

Impact: Low
Confidence: Medium

ID-2
ERC4626DelayedWithdrawalAdapter.withdrawableFunds(uint256,address) has external calls inside a loop: withdrawableAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount)

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 57 to 69 in 5342425

    
           function withdrawableFunds(uint256 positionId, address) external view override returns (uint256 withdrawableAmount) { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             // slither-disable-next-line incorrect-equality 
        
             if (requests.length == 0) { 
        
               return 0; 
        
             } 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline <= block.timestamp) { 
        
                 withdrawableAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
               } 
        
             } 
        
           }

ID-3
ERC4626DelayedWithdrawalAdapter.withdraw(uint256,address,address) has external calls inside a loop: stillPending += IERC4626(_farmToken).previewRedeem(requests[i].amount)

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 88 to 125 in 5342425

    
           function withdraw( 
        
             uint256 positionId, 
        
             address, 
        
             address recipient 
        
           ) 
        
             external 
        
             override 
        
             onlyManager 
        
             returns (uint256 withdrawn, uint256 stillPending) 
        
           { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             if (requests.length == 0) { 
        
               return (0, 0); 
        
             } 
        
             uint256 numberOfRequestsPending; 
        
             uint256 farmTokenAmountToWithdraw; 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline > block.timestamp) { 
        
                 stillPending += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
                 if (numberOfRequestsPending != i) { 
        
                   requests[numberOfRequestsPending] = requests[i]; 
        
                 } 
        
                 ++numberOfRequestsPending; 
        
               } else { 
        
                 farmTokenAmountToWithdraw += requests[i].amount; 
        
               } 
        
             } 
        
             if (numberOfRequestsPending != requests.length) { 
        
               // Resize the array 
        
               // solhint-disable-next-line no-inline-assembly 
        
               assembly { 
        
                 mstore(requests, numberOfRequestsPending) 
        
               } 
        
             } 
        
             _pendingWithdrawals[positionId] = requests; 
        
             withdrawn = IERC4626(_farmToken).redeem(farmTokenAmountToWithdraw, recipient, address(this)); 
        
           }

ID-4
ERC4626DelayedWithdrawalAdapter.estimatedPendingFunds(uint256,address) has external calls inside a loop: pendingAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount)

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 43 to 55 in 5342425

    
           function estimatedPendingFunds(uint256 positionId, address) external view override returns (uint256 pendingAmount) { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             // slither-disable-next-line incorrect-equality 
        
             if (requests.length == 0) { 
        
               return 0; 
        
             } 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline > block.timestamp) { 
        
                 pendingAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
               } 
        
             } 
        
           }

timestamp

Impact: Low
Confidence: Medium

ID-5
ERC4626DelayedWithdrawalAdapter.withdraw(uint256,address,address) uses timestamp for comparisons
Dangerous comparisons:
- requests[i].deadline > block.timestamp

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 88 to 125 in 5342425

    
           function withdraw( 
        
             uint256 positionId, 
        
             address, 
        
             address recipient 
        
           ) 
        
             external 
        
             override 
        
             onlyManager 
        
             returns (uint256 withdrawn, uint256 stillPending) 
        
           { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             if (requests.length == 0) { 
        
               return (0, 0); 
        
             } 
        
             uint256 numberOfRequestsPending; 
        
             uint256 farmTokenAmountToWithdraw; 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline > block.timestamp) { 
        
                 stillPending += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
                 if (numberOfRequestsPending != i) { 
        
                   requests[numberOfRequestsPending] = requests[i]; 
        
                 } 
        
                 ++numberOfRequestsPending; 
        
               } else { 
        
                 farmTokenAmountToWithdraw += requests[i].amount; 
        
               } 
        
             } 
        
             if (numberOfRequestsPending != requests.length) { 
        
               // Resize the array 
        
               // solhint-disable-next-line no-inline-assembly 
        
               assembly { 
        
                 mstore(requests, numberOfRequestsPending) 
        
               } 
        
             } 
        
             _pendingWithdrawals[positionId] = requests; 
        
             withdrawn = IERC4626(_farmToken).redeem(farmTokenAmountToWithdraw, recipient, address(this)); 
        
           }

ID-6
ERC4626DelayedWithdrawalAdapter.estimatedPendingFunds(uint256,address) uses timestamp for comparisons
Dangerous comparisons:
- requests[i].deadline > block.timestamp

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 43 to 55 in 5342425

    
           function estimatedPendingFunds(uint256 positionId, address) external view override returns (uint256 pendingAmount) { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             // slither-disable-next-line incorrect-equality 
        
             if (requests.length == 0) { 
        
               return 0; 
        
             } 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline > block.timestamp) { 
        
                 pendingAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
               } 
        
             } 
        
           }

ID-7
ERC4626DelayedWithdrawalAdapter.withdrawableFunds(uint256,address) uses timestamp for comparisons
Dangerous comparisons:
- requests[i].deadline <= block.timestamp

earn-periphery/src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

Lines 57 to 69 in 5342425

    
           function withdrawableFunds(uint256 positionId, address) external view override returns (uint256 withdrawableAmount) { 
        
             Request[] memory requests = _pendingWithdrawals[positionId]; 
        
             // slither-disable-next-line incorrect-equality 
        
             if (requests.length == 0) { 
        
               return 0; 
        
             } 
        
             for (uint256 i; i < requests.length; ++i) { 
        
               if (requests[i].deadline <= block.timestamp) { 
        
                 withdrawableAmount += IERC4626(_farmToken).previewRedeem(requests[i].amount); 
        
               } 
        
             } 
        
           }

naming-convention

Impact: Informational
Confidence: High

ID-8
Function ERC4626DelayedStrategy.ERC4626Vault() is not in mixedCase

earn-periphery/src/strategies/instances/erc4626/ERC4626DelayedStrategy.sol

Lines 75 to 77 in 5342425

    
           function ERC4626Vault() public view virtual override returns (IERC4626) { 
        
             return IERC4626(_farmToken); 
        
           }

0xsambugs

Great work!

0xsambugs · 2024-10-22T19:29:32Z

src/delayed-withdrawal-adapter/ERC4626DelayedWithdrawalAdapter.sol

+      }
+    }
+    _pendingWithdrawals[positionId] = requests;
+    IERC4626(_farmToken).withdraw(withdrawn, recipient, address(this));


This might fail, due to precision. I would add up all shares that are able to be withdrawn in one int, and then call redeem here (and assign he result to withdrawn)

0xsambugs · 2024-10-22T19:33:31Z

src/strategies/layers/connector/ERC4626DelayedConnector.sol

+    IERC4626 vault = ERC4626Vault();
+    // Note: we assume params are consistent and valid because they were validated by the EarnVault
+    IERC20(address(vault)).safeTransfer(
+      address(_connector_delayedWithdrawalAdapter(tokens[0])), vault.previewWithdraw(toWithdraw[0])
+    );
+
+    _connector_delayedWithdrawalAdapter(tokens[0]).initiateDelayedWithdrawal(positionId, tokens[0], toWithdraw[0]);
+
+    withdrawalTypes = _connector_supportedWithdrawals();
+  }


Correct me if I'm wrong, but I think you also need to pass the amount of shares (not assets) to initiateDelayedWithdrawal

Suggested change

IERC4626 vault = ERC4626Vault();

// Note: we assume params are consistent and valid because they were validated by the EarnVault

IERC20(address(vault)).safeTransfer(

address(_connector_delayedWithdrawalAdapter(tokens[0])), vault.previewWithdraw(toWithdraw[0])

);

_connector_delayedWithdrawalAdapter(tokens[0]).initiateDelayedWithdrawal(positionId, tokens[0], toWithdraw[0]);

withdrawalTypes = _connector_supportedWithdrawals();

}

IERC4626 vault = ERC4626Vault();

// Note: we assume params are consistent and valid because they were validated by the EarnVault

uint256 shares = vault.previewWithdraw(toWithdraw[0]);

IERC20(address(vault)).safeTransfer(

address(_connector_delayedWithdrawalAdapter(tokens[0])), shares)

);

_connector_delayedWithdrawalAdapter(tokens[0]).initiateDelayedWithdrawal(positionId, tokens[0], shares);

withdrawalTypes = _connector_supportedWithdrawals();

}

0xKoaj requested a review from 0xsambugs October 22, 2024 19:05

0xsambugs approved these changes Oct 22, 2024

View reviewed changes

0xKoaj added 5 commits October 24, 2024 10:22

feat: add erc4626 dummy delayed strategy

4880807

refactor: use erc4626 amount instead of asset in adapter

431102d

refactor: some suggestions

6b1162f

refactor: some suggestions

4c113ef

fix: return adapter

141976b

0xKoaj force-pushed the feat/bly-3229 branch from 6355291 to 141976b Compare October 24, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add erc4626 dummy delayed strategy#89

feat: add erc4626 dummy delayed strategy#89
0xKoaj wants to merge 5 commits intomainfrom
feat/bly-3229

0xKoaj commented Oct 22, 2024

Uh oh!

linear bot commented Oct 22, 2024

Uh oh!

github-actions bot commented Oct 22, 2024 •

edited

Loading

Uh oh!

0xsambugs left a comment

Uh oh!

0xsambugs Oct 22, 2024

Uh oh!

0xsambugs Oct 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

0xKoaj commented Oct 22, 2024

Uh oh!

linear bot commented Oct 22, 2024

Uh oh!

github-actions bot commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Slither report

missing-zero-check

calls-loop

timestamp

naming-convention

Uh oh!

0xsambugs left a comment

Choose a reason for hiding this comment

Uh oh!

0xsambugs Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

0xsambugs Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Oct 22, 2024 •

edited

Loading