arithmetic series curve PoC#118
Conversation
Summary of Test Results if Merged To Main:
✅ All 109 tests passed! (0 skipped, Total: 109) Test Results for Merge
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToShares(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#126-166) performs a multiplication on the result of a division: - S = totalShares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#135) - b = 2 * BASE_PRICE - priceIncrement + 2 * S * priceIncrement (src/ArithmeticSeriesCurve.sol#141)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2222-2234) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2225) Affected Files:
Recommended Actions
⛽ Gas Analysis📊 First gas snapshot created |
|
Some math analysis: Mathematical FormulationsArithmeticSeriesCurve
ProgressiveCurve
Key Differences
Are They Equivalent?They are conceptually similar but not mathematically identical: If we set
The difference is the Practical Implications
The curves are similar in concept but will produce slightly different pricing outcomes due to these fundamental mathematical differences. |
Summary of Test Results if Merged To Main:
✅ All 109 tests passed! (0 skipped, Total: 109) Test Results for Merge
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToAssets(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#208-244) performs a multiplication on the result of a division: - sharesToRedeem = shares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#224) - assets = (sharesToRedeem * (highestTerm + lowestTerm)) / 2 (src/ArithmeticSeriesCurve.sol#241)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2222-2234) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2225) Affected Files:
Recommended Actions
⛽ Gas Analysis📊 First gas snapshot created |
|
@mihailo-maksa I added some additional Natspec comments to help the auto-generated documentation, and updated the documentation script and deployment script to include all the curves. What do you think about completing the remaining preview function |
Summary of Test Results if Merged To Main:
✅ All 109 tests passed! (0 skipped, Total: 109) Test Results for Merge
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToAssets(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#208-244) performs a multiplication on the result of a division: - sharesToRedeem = shares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#224) - assets = (sharesToRedeem * (highestTerm + lowestTerm)) / 2 (src/ArithmeticSeriesCurve.sol#241)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2222-2234) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2225) Affected Files:
Recommended Actions
⛽ Gas Analysis📊 First gas snapshot created |
Signed-off-by: Mihailo Maksa <mihajlomaksa9+@gmail.com>
Summary of Test Results if Merged To Main:
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToAssets(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#208-244) performs a multiplication on the result of a division: - sharesToRedeem = shares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#224) - assets = (sharesToRedeem * (highestTerm + lowestTerm)) / 2 (src/ArithmeticSeriesCurve.sol#241)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2190-2202) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2193) Affected Files:
Recommended Actions
⛽ Gas Analysis |
| uint256 assets, // number of assets the user wants to deposit (18-decimal) | ||
| uint256, /*totalAssets*/ | ||
| uint256 totalShares // totalShares in the vault (18-decimal) |
There was a problem hiding this comment.
We probably want to use natspec instead of inline commenting for these
| uint256 shares, // number of shares the user wants to burn (18-decimal) | ||
| uint256 totalShares, // totalShares in the vault (18-decimal) | ||
| uint256 /* totalAssets */ |
There was a problem hiding this comment.
We probably want to use natspec instead of inline commenting for these
| uint256 public bondingCurveId; | ||
| uint256 public bondingCurveId2; | ||
|
|
||
| function setUp() external { |
There was a problem hiding this comment.
would be really nice to have a shared "default" setUp function to cut down on LOC
There was a problem hiding this comment.
Yeah, we could definitely work on that
| (uint256 userShares,) = ethMultiVault.getVaultStateForUserCurve(atomId, curveId, user); | ||
| assertEq(userShares, expectedUserShares); | ||
|
|
||
| // Case 2: Deposit enough to mint 1 additional share |
There was a problem hiding this comment.
Why add cases in addition to the ones parameterized by the fuzz test?
Not saying it's wrong, just curious
There was a problem hiding this comment.
That was mostly because I went about testing step by step, e.g. first make sure we can mint 1 share, then some known small number of shares (e.g. 5 more shares) and finally some bigger, more random number of shares. Same logic applies with testing redemptions as well.
Although you're correct that these might not be needed anymore and removing some of these cases could simplify the test suite
|
Looks really good! Some minor natspec nits and questions |
Summary of Test Results if Merged To Main:
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToAssets(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#208-244) performs a multiplication on the result of a division: - sharesToRedeem = shares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#224) - assets = (sharesToRedeem * (highestTerm + lowestTerm)) / 2 (src/ArithmeticSeriesCurve.sol#241)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2190-2202) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2193) Affected Files:
Recommended Actions
⛽ Gas Analysis |
Signed-off-by: Auroter <7332587+auroter@users.noreply.github.com>
Summary of Test Results if Merged To Main:
🔒 Security AnalysisHigh Severity Issuesarbitrary-send-ethImpact: 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:
View Detailed Findings
Medium Severity IssuesView Medium Severity Issues##### divide-before-multiply **Impact**: ArithmeticSeriesCurve.convertToAssets(uint256,uint256,uint256) (src/ArithmeticSeriesCurve.sol#208-244) performs a multiplication on the result of a division: - sharesToRedeem = shares / DECIMAL_PRECISION (src/ArithmeticSeriesCurve.sol#224) - assets = (sharesToRedeem * (highestTerm + lowestTerm)) / 2 (src/ArithmeticSeriesCurve.sol#241)Affected Files:
incorrect-equalityImpact: EthMultiVault._validateTimelock(bytes32) (src/EthMultiVault.sol#2190-2202) uses a dangerous strict equality: - timelock.readyTime == 0 (src/EthMultiVault.sol#2193) Affected Files:
Recommended Actions
⛽ Gas Analysis |
3 participants
Overview
This PR introduces a new bonding curve implementation, the
ArithmeticSeriesCurve, which provides a simple, incremental price increase for each subsequent share minted, and the similar mechanism for each share redeemed. The design aims to be intuitive, “just work,” and serve as a robust starting point for testing different economic models—ranging from more to less aggressive price increments.Key Features
Simplicity & Robustness:
User-Friendly & Transparent:
Enhanced Security Against Frontrunning:
Comprehensive Testing:
Limitations & Future Work
Whole-Number Only:
Fee Handling:
protocolFee,entryFee, andexitFeeset to zero. When fees are applied, the returned share amounts become fractional in many cases as these fees are proportionate (percentage-based).In conclusion, future iterations will explore strategies to integrate fee mechanisms in an efficient manner and without causing any fractional share issues, if the overall concept of the
ArithmeticSeriesCurveis accepted and is determined that it's worth investing additional effort in doing so.