Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sean/fea urchin #112

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Sean/fea urchin #112

wants to merge 6 commits into from

Conversation

auroter
Copy link
Member

@auroter auroter commented Feb 12, 2025

FeaUrchin Contract Implementation

Overview

This PR introduces the FeaUrchin contract, a fee-taking wrapper around the EthMultiVault that adds a configurable fee structure for all vault operations. The implementation includes support for both standard and bonding curve operations, and provides an interface which is forward compatible by abstracting out "curve" functions, as well as term abstraction for atoms/triples. The purpose of this is for app developers to be able to charge fees, for coffee, or to fund their back-end wallets. There is a new template project in the monorepo which will certainly use this.

Key Changes

New Contract: FeaUrchin

  • Implements a fee-taking wrapper around EthMultiVault operations
  • Configurable fee structure with numerator/denominator pattern
  • Tracks unique users, total assets moved, and fees collected
  • Supports both single and batch operations
  • Handles both standard and bonding curve operations

Interface Updates

  • Added bonding curve support to IEthMultiVault interface (This was missing)
  • New methods for curve-based deposits and redemptions for both atoms and triples
  • Added NATSPEC documentation for all new methods

Test Coverage

  • Comprehensive test suite in FeaUrchin.t.sol
  • Tests for initialization, fee configuration, atom/triple creation
  • Batch operation testing
  • Fee collection and withdrawal testing

Implementation Details

Fee Handling

  • Fees are calculated using the formula: fee = amount * feeNumerator / feeDenominator
  • Default configuration: 1% fee (100/10000)
  • Fees are collected in ETH and can be withdrawn by the admin

Asset Tracking

  • totalAssetsMoved: Total value of assets that have moved through the contract
  • totalAssetsStaked: Current total value of assets staked
  • totalFeesCollected: Total fees collected by the contract
  • uniqueUsersCount: Number of unique users interacting with the contract

Batch Operations

  • Support for batch creation of atoms and triples
  • Batch deposits and redemptions with curve specification
  • Efficient fee handling for batch operations

Testing

All core functionality has been tested, including:

  • Contract initialization
  • Fee calculations and collection
  • Asset tracking
  • User tracking
  • Batch operations
  • Integration with bonding curves

Security Considerations

  • Owner-only access for fee configuration and withdrawal
  • Safe ETH transfer handling
  • Maintains emergency redemption capabilities from EthMultiVault
  • Preserves all existing security checks from the base contract

Commits

  1. Basic implementation
  2. Fix everything, add term abstraction, curve abstraction, batch actions
  3. Remove unused events
  4. Add atomCost and tripleCost overrides to account for fees, fix redeem pass-through

Note:

WE DO NOT WANT TO MERGE THIS YET BECAUSE THE MAIN BRANCH IS BEING AUDITED. This PR is for educational purposes, for now.

@github-actions GitHub Actions
Copy link

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

✅ All 115 tests passed! (0 skipped, Total: 115)

Test Results for Merge

Test Suite Status Coverage Time
test/unit/EthMultiVault/RedeemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/RedeemAtomCurve.t.sol ✅ 100% (4/4) 0.004s
test/unit/EthMultiVault/AdminMultiVault.t.sol ✅ 100% (16/16) 0.008s
test/unit/EthMultiVault/Approvals.t.sol ✅ 100% (2/2) 0.004s
test/unit/EthMultiVault/RedeemTriple.t.sol ✅ 100% (5/5) 0.008s
test/unit/EthMultiVault/RedeemTripleCurve.t.sol ✅ 100% (5/5) 0.017s
test/unit/EthMultiVault/CreateTriple.t.sol ✅ 100% (6/6) 0.006s
test/BaseTest.sol ✅ 100% (2/2) 0.012s
test/unit/EthMultiVault/DepositAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/BatchCreateAtom.t.sol ✅ 100% (2/2) 0.004s
test/unit/EthMultiVault/DepositAtomCurve.t.sol ✅ 100% (4/4) 0.004s
test/unit/EthMultiVault/DepositTriple.t.sol ✅ 100% (4/4) 0.007s
test/unit/EthMultiVault/BatchCreateTriple.t.sol ✅ 100% (4/4) 0.014s
test/unit/EthMultiVault/CreateAtom.t.sol ✅ 100% (6/6) 0.003s
test/unit/EthMultiVault/DepositTripleCurve.t.sol ✅ 100% (4/4) 0.014s
test/unit/EthMultiVault/EmergencyRedeemTriple.t.sol ✅ 100% (5/5) 0.008s
test/unit/EthMultiVault/EmergencyReedemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/Helpers.t.sol ✅ 100% (4/4) 0.004s
test/unit/EthMultiVault/UseCases.t.sol ✅ 100% (6/6) 0.060s
test/unit/EthMultiVault/Profit.t.sol ✅ 100% (11/11) 0.031s
test/unit/FeaUrchin.t.sol ✅ 100% (8/8) 0.009s

🔒 Security Analysis

⚠️ Found 1 High and 2 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2214-2226) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2217)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2214 in _validateTimelock

reentrancy-no-eth

Impact: Reentrancy in FeaUrchin.batchRedeem(uint256[],address,uint256[],uint256[]) (src/FeaUrchin.sol#219-241): External calls: - assets[i] = _redeem(shares[i],ids[i],curveIds[i]) (src/FeaUrchin.sol#232) - assets = ethMultiVault.redeemTriple(shares,address(this),id) (src/FeaUrchin.sol#145-147) - assets = ethMultiVault.redeemAtom(shares,address(this),id) (src/FeaUrchin.sol#145-147) - assets = ethMultiVault.redeemTripleCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#149-151) - assets = ethMultiVault.redeemAtomCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#149-151) State variables written after the call(s): - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalAssetsMoved += assets (src/FeaUrchin.sol#76) FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#24) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#24) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalAssetsStaked -= assets (src/FeaUrchin.sol#77) FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#25) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#25) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalFeesCollected += fee (src/FeaUrchin.sol#78) FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#26) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#26)

Affected Files:

  • src/FeaUrchin.sol

  • src/FeaUrchin.sol:219 in batchRedeem

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

📊 First gas snapshot created

@github-actions GitHub Actions
Copy link

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

✅ All 117 tests passed! (0 skipped, Total: 117)

Test Results for Merge

Test Suite Status Coverage Time
test/unit/EthMultiVault/RedeemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/AdminMultiVault.t.sol ✅ 100% (16/16) 0.006s
test/unit/EthMultiVault/RedeemAtomCurve.t.sol ✅ 100% (4/4) 0.004s
test/unit/EthMultiVault/Approvals.t.sol ✅ 100% (2/2) 0.004s
test/unit/EthMultiVault/RedeemTriple.t.sol ✅ 100% (5/5) 0.009s
test/BaseTest.sol ✅ 100% (2/2) 0.011s
test/unit/EthMultiVault/RedeemTripleCurve.t.sol ✅ 100% (5/5) 0.011s
test/unit/EthMultiVault/BatchCreateAtom.t.sol ✅ 100% (2/2) 0.008s
test/unit/EthMultiVault/BatchCreateTriple.t.sol ✅ 100% (4/4) 0.010s
test/unit/EthMultiVault/CreateAtom.t.sol ✅ 100% (6/6) 0.007s
test/unit/EthMultiVault/CreateTriple.t.sol ✅ 100% (6/6) 0.008s
test/unit/EthMultiVault/DepositAtom.t.sol ✅ 100% (4/4) 0.006s
test/unit/EthMultiVault/Helpers.t.sol ✅ 100% (4/4) 0.005s
test/unit/EthMultiVault/DepositAtomCurve.t.sol ✅ 100% (4/4) 0.009s
test/unit/EthMultiVault/UseCases.t.sol ✅ 100% (6/6) 0.045s
test/unit/EthMultiVault/DepositTriple.t.sol ✅ 100% (4/4) 0.011s
test/unit/EthMultiVault/Profit.t.sol ✅ 100% (11/11) 0.029s
test/unit/EthMultiVault/DepositTripleCurve.t.sol ✅ 100% (4/4) 0.010s
test/unit/FeaUrchin.t.sol ✅ 100% (8/8) 0.007s
test/unit/EthMultiVault/EmergencyReedemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/EmergencyRedeemTriple.t.sol ✅ 100% (5/5) 0.008s

🔒 Security Analysis

⚠️ Found 1 High and 2 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2214-2226) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2217)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2214 in _validateTimelock

reentrancy-no-eth

Impact: Reentrancy in FeaUrchin.batchRedeem(uint256[],address,uint256[],uint256[]) (src/FeaUrchin.sol#219-241): External calls: - assets[i] = _redeem(shares[i],ids[i],curveIds[i]) (src/FeaUrchin.sol#232) - assets = ethMultiVault.redeemTriple(shares,address(this),id) (src/FeaUrchin.sol#145-147) - assets = ethMultiVault.redeemAtom(shares,address(this),id) (src/FeaUrchin.sol#145-147) - assets = ethMultiVault.redeemTripleCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#149-151) - assets = ethMultiVault.redeemAtomCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#149-151) State variables written after the call(s): - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalAssetsMoved += assets (src/FeaUrchin.sol#76) FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#24) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#24) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalAssetsStaked -= assets (src/FeaUrchin.sol#77) FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#25) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#25) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#233) - totalFeesCollected += fee (src/FeaUrchin.sol#78) FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#26) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#66-72) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#74-80) - FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#26)

Affected Files:

  • src/FeaUrchin.sol

  • src/FeaUrchin.sol:219 in batchRedeem

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

⚠️ No gas snapshot generated

@github-actions GitHub Actions
Copy link

Summary of Test Results if Merged To Main:

  • Full logs & artifacts are available in the Actions tab
  • This comment will update automatically with new CI runs

✅ All 117 tests passed! (0 skipped, Total: 117)

Test Results for Merge

Test Suite Status Coverage Time
test/unit/EthMultiVault/RedeemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/EthMultiVault/RedeemAtomCurve.t.sol ✅ 100% (4/4) 0.004s
test/unit/EthMultiVault/AdminMultiVault.t.sol ✅ 100% (16/16) 0.011s
test/unit/EthMultiVault/Approvals.t.sol ✅ 100% (2/2) 0.001s
test/unit/EthMultiVault/CreateTriple.t.sol ✅ 100% (6/6) 0.014s
test/unit/EthMultiVault/RedeemTriple.t.sol ✅ 100% (5/5) 0.008s
test/unit/EthMultiVault/DepositAtom.t.sol ✅ 100% (4/4) 0.010s
test/BaseTest.sol ✅ 100% (2/2) 0.016s
test/unit/EthMultiVault/RedeemTripleCurve.t.sol ✅ 100% (5/5) 0.014s
test/unit/EthMultiVault/BatchCreateAtom.t.sol ✅ 100% (2/2) 0.008s
test/unit/EthMultiVault/DepositAtomCurve.t.sol ✅ 100% (4/4) 0.014s
test/unit/EthMultiVault/DepositTriple.t.sol ✅ 100% (4/4) 0.007s
test/unit/EthMultiVault/BatchCreateTriple.t.sol ✅ 100% (4/4) 0.016s
test/unit/EthMultiVault/CreateAtom.t.sol ✅ 100% (6/6) 0.003s
test/unit/EthMultiVault/DepositTripleCurve.t.sol ✅ 100% (4/4) 0.012s
test/unit/EthMultiVault/EmergencyReedemAtom.t.sol ✅ 100% (4/4) 0.003s
test/unit/FeaUrchinFactory.t.sol ✅ 100% (2/2) 0.001s
test/unit/EthMultiVault/EmergencyRedeemTriple.t.sol ✅ 100% (5/5) 0.015s
test/unit/FeaUrchin.t.sol ✅ 100% (8/8) 0.007s
test/unit/EthMultiVault/Helpers.t.sol ✅ 100% (4/4) 0.005s
test/unit/EthMultiVault/Profit.t.sol ✅ 100% (11/11) 0.025s
test/unit/EthMultiVault/UseCases.t.sol ✅ 100% (6/6) 0.084s

🔒 Security Analysis

⚠️ Found 1 High and 2 Medium severity issues

High Severity Issues

arbitrary-send-eth

Impact: AtomWallet._call(address,uint256,bytes) (src/AtomWallet.sol#214-221) sends eth to arbitrary user Dangerous calls: - (success,result) = target.call{value: value}(data) (src/AtomWallet.sol#215)

Affected Files:

  • src/AtomWallet.sol
View Detailed Findings
  • src/AtomWallet.sol:214 in _call

Medium Severity Issues

View Medium Severity Issues ##### incorrect-equality **Impact**: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2214-2226) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2217)

Affected Files:

  • src/EthMultiVault.sol

  • src/EthMultiVault.sol:2214 in _validateTimelock

reentrancy-no-eth

Impact: Reentrancy in FeaUrchin.batchRedeem(uint256[],address,uint256[],uint256[]) (src/FeaUrchin.sol#367-389): External calls: - assets[i] = _redeem(shares[i],ids[i],curveIds[i]) (src/FeaUrchin.sol#380) - assets = ethMultiVault.redeemTriple(shares,address(this),id) (src/FeaUrchin.sol#267-269) - assets = ethMultiVault.redeemAtom(shares,address(this),id) (src/FeaUrchin.sol#267-269) - assets = ethMultiVault.redeemTripleCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#271-273) - assets = ethMultiVault.redeemAtomCurve(shares,address(this),id,curveId) (src/FeaUrchin.sol#271-273) State variables written after the call(s): - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#381) - totalAssetsMoved += assets (src/FeaUrchin.sol#165) FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#84) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#151-157) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#163-169) - FeaUrchin.totalAssetsMoved (src/FeaUrchin.sol#84) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#381) - totalAssetsStaked -= assets (src/FeaUrchin.sol#166) FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#86) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#151-157) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#163-169) - FeaUrchin.totalAssetsStaked (src/FeaUrchin.sol#86) - (fee,netValue) = _processRedeem(assets[i]) (src/FeaUrchin.sol#381) - totalFeesCollected += fee (src/FeaUrchin.sol#167) FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#88) can be used in cross function reentrancies: - FeaUrchin._processDeposit(uint256) (src/FeaUrchin.sol#151-157) - FeaUrchin._processRedeem(uint256) (src/FeaUrchin.sol#163-169) - FeaUrchin.totalFeesCollected (src/FeaUrchin.sol#88)

Affected Files:

  • src/FeaUrchin.sol

  • src/FeaUrchin.sol:367 in batchRedeem

Recommended Actions

  1. Review and fix all high severity issues before deployment
  2. Implement thorough testing for affected components
  3. Consider additional security measures:
    • Access controls
    • Input validation
    • Invariant checks

⛽ Gas Analysis

📊 First gas snapshot created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihailo-maksa mihailo-maksa Awaiting requested review from mihailo-maksa

@mcclurejt mcclurejt Awaiting requested review from mcclurejt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@auroter