Runtime fv #30
Runtime fv #30
Conversation
miscellaneous audit fixes
delete sweep assets and integrate token transfer into redeem repo tokens
cache total asset value
sweep if funds freed
Refactor to use repo token utils
fixes getNormalized repo token amount to only apply redemption haircu…
get rid of unneccessary looping for present value of single token
Refactor pending offer memory
WalkthroughThe changes encompass significant modifications across multiple Solidity files, focusing on enhancing the handling of repo tokens, refining calculations, and introducing new parameters for dynamic discount rate management. Key updates include the addition of a discount rate adapter, the removal of certain utility functions, and adjustments to function visibility and logic for improved clarity and efficiency. New event emissions and access control features are also introduced to enhance contract functionality and security. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/interfaces/term/ITermDiscountRateAdapter.sol (1)
5-5: LGTM! Consider adding documentation for clarity.The addition of the
repoRedemptionHaircutfunction to theITermDiscountRateAdapterinterface is consistent with the existing code and follows the Solidity syntax and naming conventions.However, to improve the clarity and maintainability of the code, consider adding documentation or comments to explain the purpose and usage of the
repoRedemptionHaircutfunction. Clarify how the haircut value relates to the discount rate and provide any necessary context or guidelines for using this function effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- src/RepoTokenList.sol (5 hunks)
- src/RepoTokenUtils.sol (1 hunks)
- src/Strategy.sol (18 hunks)
- src/TermAuctionList.sol (6 hunks)
- src/TermDiscountRateAdapter.sol (2 hunks)
- src/TermVaultEventEmitter.sol (1 hunks)
- src/interfaces/term/ITermDiscountRateAdapter.sol (1 hunks)
- src/interfaces/term/ITermVaultEvents.sol (1 hunks)
- src/periphery/StrategyAprOracle.sol (1 hunks)
- src/test/TestUSDCOffers.t.sol (1 hunks)
- src/test/TestUSDCSellRepoToken.t.sol (1 hunks)
- src/test/TestUSDCSubmitOffer.t.sol (1 hunks)
- src/test/mocks/MockTermAuctionOfferLocker.sol (2 hunks)
- src/test/utils/Setup.sol (2 hunks)
Additional comments not posted (35)
src/interfaces/term/ITermVaultEvents.sol (1)
5-5: LGTM!The
VaultContractPairedevent is a valuable addition to theITermVaultEventsinterface. It allows external listeners to be notified when a vault contract is successfully paired with the term vault. The event name and parameter are clear and descriptive, making it easy for developers to understand and use.src/TermDiscountRateAdapter.sol (5)
7-7: LGTM!The import of
AccessControlUpgradeableis necessary and the import path is correct.
14-16: LGTM!
- Inheriting from
AccessControlUpgradeableis necessary to add access control functionality.- Defining the
ORACLE_ROLEconstant is a good practice and usingkeccak256hash ensures uniqueness.
19-19: LGTM!The public mapping
repoRedemptionHaircutis well-defined with appropriate types and naming convention.
24-28: LGTM!
- The constructor now correctly takes the
oracleWallet_parameter to set the oracle wallet address.- Granting the
ORACLE_ROLEto theoracleWallet_address ensures proper access control.
46-54: LGTM!
- The
setRepoRedemptionHaircutfunction is implemented correctly with proper access control using theonlyRole(ORACLE_ROLE)modifier.- The function updates the
repoRedemptionHaircutmapping as expected.- The function is well-documented with NatSpec comments.
src/RepoTokenUtils.sol (1)
47-62: Verify the impact of therepoRedemptionHaircutparameter on the callers.The addition of the
repoRedemptionHaircutparameter and the conditional logic to apply the haircut is a significant change that allows for more nuanced handling of repoToken values based on the haircut. The change is implemented correctly. However, it's important to ensure that the new parameter is being passed correctly from all the callers of this function and that the implications of this change are handled appropriately in the rest of the system.Run the following script to verify the usage of the
getNormalizedRepoTokenAmountfunction:Verification successful
Verification successful:
repoRedemptionHaircutparameter is correctly passed in all function calls.The
getNormalizedRepoTokenAmountfunction is consistently called with therepoRedemptionHaircutparameter across multiple files, confirming that the integration of this parameter is complete and correct. No issues were found in the function calls.
- Files verified:
src/TermAuctionList.sol,src/Strategy.sol,src/RepoTokenList.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getNormalizedRepoTokenAmount` pass the new parameter. # Test: Search for the function usage. Expect: All occurrences pass the new parameter. rg --type solidity -A 5 $'getNormalizedRepoTokenAmount'Length of output: 87
Script:
#!/bin/bash # Description: Verify all function calls to `getNormalizedRepoTokenAmount` pass the new parameter. # Test: Search for the function usage in Solidity files by looking for the function name. rg 'getNormalizedRepoTokenAmount' --glob '*.sol' -A 5Length of output: 3884
src/TermVaultEventEmitter.sol (1)
36-36: LGTM!The addition of the
VaultContractPairedevent emission is a good enhancement to the contract's functionality. It provides a mechanism to notify external listeners about the pairing of a vault contract, which can be useful for tracking and auditing purposes. The event is correctly emitted after granting theVAULT_CONTRACTrole to thevaultContractaddress, and the event name and emitted argument are appropriate and descriptive.src/test/mocks/MockTermAuctionOfferLocker.sol (2)
44-44: LGTM!Returning
auction.auctionEndTime()in therevealTimefunction is a valuable addition. It provides the necessary auction timing information that was previously missing, which is crucial for determining when bids can be revealed. This change enhances the functionality and usability of the contract.
28-28: Verify the intention and analyze the impact of settingauctionStartTimeto0.Setting
auctionStartTimeto a fixed value of0instead ofblock.timestampseems intentional based on the PR title. However, this change could impact the auction timing logic that depends onauctionStartTime.Please verify the intention behind this change and analyze its impact on the auction timing logic, specifically:
- How does setting
auctionStartTimeto0affect the start and end times of the auction?- Are there any functions or external contracts that rely on
auctionStartTimebeing set to the actual deployment time?- Have the necessary adjustments been made to accommodate this change in the rest of the codebase?
src/test/utils/Setup.sol (2)
53-53: NeworacleWalletvariable looks good, but clarify the address value.The addition of the
oracleWalletvariable is fine for a test setup contract. However, could you please clarify the significance of the hardcoded address valueaddress(333)? Is it a placeholder or a specific oracle address?
99-99:TermDiscountRateAdapterconstructor change looks good, but verify the contract changes.Passing the
oracleWalletto theTermDiscountRateAdapterconstructor is consistent with the addition of theoracleWalletvariable. However, make sure corresponding changes have been made within theTermDiscountRateAdaptercontract to actually utilize this neworacleWalletparameter for interacting with the oracle.Run the following script to verify the
TermDiscountRateAdaptercontract changes:Verification successful
oracleWalletparameter is utilized inTermDiscountRateAdapter.The
oracleWalletparameter is used in the constructor of theTermDiscountRateAdaptercontract to grant theORACLE_ROLE, confirming its integration into the contract's functionality. No further changes are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `TermDiscountRateAdapter` contract utilizes the new `oracleWallet` parameter. # Test: Search for `oracleWallet` usage within `TermDiscountRateAdapter`. # Expect: At least one occurrence of `oracleWallet` being used. rg --type solidity -A 5 $'oracleWallet' $(fd TermDiscountRateAdapter.sol)Length of output: 186
Script:
#!/bin/bash # Description: Verify `TermDiscountRateAdapter` contract utilizes the new `oracleWallet` parameter. # Locate the TermDiscountRateAdapter.sol file and search for oracleWallet usage. fd TermDiscountRateAdapter.sol --exec rg 'oracleWallet' {}Length of output: 237
src/test/TestUSDCOffers.t.sol (2)
40-40: LGTM!Setting the
requiredReserveRatioto0in the test setup allows for a more flexible approach to collateral management during testing. The change is made by the authorizedmanagementaccount.
41-41: LGTM!Setting the
discountRateMarkupto0in the test setup implies that offers are made without additional cost during testing. The change is made by the authorizedmanagementaccount.src/test/TestUSDCSubmitOffer.t.sol (2)
40-40: Ensure the reserve ratio is not set to 0 in the actual contract.Setting the required reserve ratio to 0 in the test setup is fine for simplifying the testing environment. However, please verify that the reserve ratio is not set to 0 in the actual
Strategycontract, as that would eliminate the safety buffer usually maintained by the contract.Run the following script to verify the reserve ratio in the actual contract:
41-41: Ensure the discount rate markup is not set to 0 in the actual contract.Setting the discount rate markup to 0 in the test setup is fine for simplifying the testing environment. However, please verify that the markup is not set to 0 in the actual
Strategycontract, as that could impact the pricing strategies and profitability of the contract.Run the following script to verify the discount rate markup in the actual contract:
src/TermAuctionList.sol (4)
97-143: LGTM!The function correctly inserts a new pending offer into the list data while maintaining the list sorted by auction address. The logic handles all the edge cases and updates the pointers and mappings accordingly.
194-194: LGTM!The function correctly unlocks offers when an auction is canceled for withdrawal. The logic passes the correct
offerIdsto theunlockOffersfunction and the comment accurately describes the behavior.
Line range hint
251-286: LGTM!The function correctly handles the edge case where the offer is processed, but
auctionClosedis not yet called and the auction is new. It checks therepoTokendiscountRatesto avoid double counting on re-openings and marks the edge case auction as processed to avoid double counting. The logic for calculating therepoTokenAmountInBaseAssetPrecisionand the present value is correct.
325-357: LGTM!The function correctly handles the edge case where the offer is processed, but
auctionClosedis not yet called and the auction is new. It checks therepoTokendiscountRatesto avoid double counting on re-openings and marks the edge case auction as processed to avoid double counting. The logic for calculating theofferAmountusing thegetNormalizedRepoTokenAmountfunction is correct.src/RepoTokenList.sol (2)
Line range hint
116-156: LGTM!The addition of the
discountRateAdapterparameter and the refactoring to useRepoTokenUtils.getNormalizedRepoTokenAmountare great improvements. They enhance the flexibility and readability of the function.
176-200: LGTM!The changes to the
getPresentValuefunction are excellent. The addition of thediscountRateAdapterparameter, the removal of the filtering logic, and the refactoring to useRepoTokenUtils.getNormalizedRepoTokenAmountall contribute to making the function more flexible, efficient, and readable.src/test/TestUSDCSellRepoToken.t.sol (1)
52-52: SettingrequiredReserveRatioto0could lead to liquidity issues.Setting the
requiredReserveRatioto0implies that no reserves are required at all. While this allows for more flexible liquidity management, it could lead to issues if there are large, sudden withdrawals. The system may not have sufficient liquidity to handle such scenarios.To verify the impact, run this script to analyze how the system behaves under different withdrawal scenarios with
requiredReserveRatioset to0:The key aspects to verify are:
- How the system liquidity is impacted after large withdrawals.
- Whether there are any checks on
requiredReserveRatiobefore allowing withdrawals.If the liquidity drops too low or there are no checks, it indicates high severity issues that need to be addressed.
src/Strategy.sol (12)
56-56: Verify the usage and impact of the newdepositLockvariable.The addition of the
depositLockstate variable looks good. However, please ensure that:
- The variable is properly utilized within the contract to control deposit operations as intended.
- The impact of this lock on the contract's behavior, especially on the
depositand related functions, is thoroughly tested.
347-347: LGTM!Reverting with a custom error when the
repoTokenis invalid is a good practice. It makes the error more specific and can save gas.
354-361: Great refactoring!Utilizing the
RepoTokenUtils.getNormalizedRepoTokenAmountfunction to calculate therepoTokenAmountInBaseAssetPrecisionis a nice refactoring. It improves code clarity and reduces redundancy. The calculation also seems correct as it takes into account all the necessary parameters.
400-400: Verify the reasoning behind the visibility change.Changing the visibility of
calculateRepoTokenPresentValuefromexternaltopubliclooks fine. However, please ensure that:
- The function indeed needs to be called internally within the contract, justifying the visibility change.
- The impact of this change on the contract's behavior and access control is thoroughly considered and tested.
403-409: Looks good!Retrieving the
redemptionTimestampfrom therepoToken's config and usingRepoTokenUtils.getNormalizedRepoTokenAmountfor normalization are both good practices. They ensure correctness and improve code quality. The calculation seems accurate as well.
431-438: Calculation looks correct!The calculation of
repoTokenHoldingPVis properly guarded by the check on thediscountRatesmapping, avoiding unnecessary computations. The usage ofcalculateRepoTokenPresentValuewith the correct parameters ensures an accurate and consistent calculation of the present value.
Line range hint
439-445: Comprehensive valuation!Returning the sum of
repoTokenHoldingPVand the present value fromtermAuctionListDataprovides a complete valuation of therepoTokenholdings. The usage ofgetPresentValuewith the appropriate parameters ensures an accurate calculation. This change enhances the contract's functionality in line with the summary.
497-498: Accurate present value calculation!The usage of
repoTokenListData.getPresentValuewith the correct parameters to calculate the present value of all repoTokens is accurate. This change contributes to the correct determination of the total asset value.
Line range hint
604-609: Correct retrieval of cumulative repo token data!The usage of
repoTokenListData.getCumulativeRepoTokenDatawith the appropriate parameters to retrieve the cumulative repo token data is correct. This change is crucial for accurately calculating the weighted time to maturity.
Line range hint
620-625: Accurate retrieval of cumulative offer data!The usage of
termAuctionListData.getCumulativeOfferDatawith the appropriate parameters to retrieve the cumulative offer data is accurate. This change is vital for correctly calculating the weighted time to maturity, taking into account both repo tokens and auction offers.
635-642: Proper handling of new repoToken!The code block correctly handles the case when a new
repoTokenis being considered for the weighted time to maturity calculation. TherepoTokenAmountInBaseAssetPrecisionis accurately calculated usingRepoTokenUtils.getNormalizedRepoTokenAmount, and the cumulative values are updated accordingly. This change ensures that new repoTokens are properly accounted for in the calculation.
692-693: Correct removal of completed auction offers!The usage of
termAuctionListData.removeCompletedwith the appropriate parameters to remove completed auction offers is correct. This change is crucial for maintaining an accurate state and aligns with the summary. It ensures that thetermAuctionListDatais properly updated based on the currentrepoTokenListData,discountRateAdapter, and the asset address.
| @@ -28,7 +28,7 @@ contract StrategyAprOracle is AprOracleBase { | |||
| function aprAfterDebtChange( | |||
| address _strategy, | |||
| int256 _delta | |||
| ) external view override returns (uint256) { | |||
| ) external pure override returns (uint256) { | |||
There was a problem hiding this comment.
Implement the actual logic, consider changing the modifier, and add input validation.
The change from view to pure aligns with the function's current behavior of returning a fixed value. However, please consider the following suggestions:
-
Implement the actual logic to return the most accurate APR estimation for the strategy as indicated by the TODO comment. The current implementation with a fixed return value is likely a placeholder.
-
Once the actual logic is implemented, consider changing the modifier back to
viewif the function needs to read from the contract's state. -
Add input validation to ensure that
_strategyis a valid address and_deltais within an acceptable range. This will help prevent unexpected behavior and potential vulnerabilities.
Do you want me to assist with implementing the actual logic or open a GitHub issue to track this task?
| @@ -49,6 +49,8 @@ contract TestUSDCSellRepoToken is Setup { | |||
| termStrategy.setCollateralTokenParams(address(mockCollateral), 0.5e18); | |||
| termStrategy.setTimeToMaturityThreshold(10 weeks); | |||
| termStrategy.setRepoTokenConcentrationLimit(1e18); | |||
| termStrategy.setRequiredReserveRatio(0); | |||
| termStrategy.setdiscountRateMarkup(0); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Setting discountRateMarkup to 0 reduces proceeds and impacts profitability.
The discountRateMarkup is used in calculating the proceeds from repo token sales. Setting it to 0 would reduce these proceeds, potentially impacting the protocol's profitability. However, no evidence was found indicating increased risk exposure from uncollateralized loans.
- File:
src/Strategy.sol- Function:
setdiscountRateMarkup
- Function:
Analysis chain
Setting discountRateMarkup to 0 could negatively impact protocol economics.
Setting the discountRateMarkup to 0 eliminates any buffer or margin for the protocol. While this could lead to more competitive pricing, it also means the protocol is taking on risk without adequate compensation. This could negatively impact the protocol's profitability and sustainability in the long run.
To verify the impact, run this script to analyze how the protocol's profitability and risk exposure are affected with discountRateMarkup set to 0:
The key aspects to verify are:
- How the effective price with
discountRateMarkupset to0compares with market prices. - Whether the protocol can issue uncollateralized loans, indicating high risk exposure.
If the effective price is consistently lower than market or uncollateralized loans are possible, it indicates issues with profitability and risk that need to be addressed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description:
# Verify the impact of setting discountRateMarkup to 0 on the protocol's profitability
# and risk exposure.
# Test 1: Search for functions that utilize discountRateMarkup in pricing
pricingFunctions=$(rg --type solidity -l -g '*.sol' -g '!test' -e 'function.*discountRateMarkup')
# For each pricing function found
for pricingFunc in $pricingFunctions; do
echo "Analyzing pricing function in $pricingFunc:"
# Find the contract name
contract=$(sed -En 's/^contract (\w+).*$/\1/p' $pricingFunc)
# Calculate the effective price with discountRateMarkup set to 0
rg --type solidity -A 10 -B 10 --context "$pricingFunc\(.*\)" $pricingFunc
# Compare with market prices to gauge competitiveness
echo "Comparing effective price with market prices..."
# (Pseudo-code as it requires external data)
# marketPrice = getMarketPrice(contract)
# if effectivePrice < marketPrice:
# print("Effective price is lower than market! Profitability may be impacted.")
done
# Test 2: Analyze risk exposure by checking for uncollateralized loans
rg --type solidity -A 10 -B 10 --context "getCollateralRatio\(.*\)" $(fd -e sol -E test)
# If collateral ratio can go below 1 (i.e., uncollateralized), flag as high risk
if [ $? -eq 0 ]; then
echo "High risk exposure: Loans can become uncollateralized!"
fi
Length of output: 886
Script:
#!/bin/bash
# Description:
# Verify the impact of setting discountRateMarkup to 0 on the protocol's profitability
# and risk exposure.
# Test 1: Search for functions that utilize discountRateMarkup in pricing
pricingFunctions=$(rg -l -g '*.sol' -g '!test' 'function.*discountRateMarkup')
# For each pricing function found
for pricingFunc in $pricingFunctions; do
echo "Analyzing pricing function in $pricingFunc:"
# Find the contract name
contract=$(sed -En 's/^contract (\w+).*$/\1/p' $pricingFunc)
# Calculate the effective price with discountRateMarkup set to 0
rg -A 10 -B 10 'discountRateMarkup' $pricingFunc
# Compare with market prices to gauge competitiveness
echo "Comparing effective price with market prices..."
# (Pseudo-code as it requires external data)
# marketPrice = getMarketPrice(contract)
# if effectivePrice < marketPrice:
# print("Effective price is lower than market! Profitability may be impacted.")
done
# Test 2: Analyze risk exposure by checking for uncollateralized loans
rg -A 10 -B 10 'getCollateralRatio\(.*\)' $(fd -e sol -E test)
# If collateral ratio can go below 1 (i.e., uncollateralized), flag as high risk
if [ $? -eq 0 ]; then
echo "High risk exposure: Loans can become uncollateralized!"
fi
Length of output: 5415
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests