feat: Implemented Djed Tefnut smart contracts with ETH backing #41
Conversation
WalkthroughAdds a new Djed-like Solidity system: DjedTefnut contract (stable/reserve coins, oracle-backed pricing, treasury fees, trading hooks), deployment parameters and script, a mock oracle for tests, and a comprehensive test suite validating buy/sell flows, fees, limits, and pricing helpers. Changes
Sequence DiagramsequenceDiagram
participant User
participant Djed as DjedTefnut
participant Oracle as IOracleShu
participant Stable as StableCoin
participant Reserve as ReserveCoin
participant Treasury
User->>Djed: buyStableCoins(receiver, feeUi, ui) {ETH}
activate Djed
Djed->>Oracle: updateOracleValues / readMaxPrice / readMinPrice
Oracle-->>Djed: priceHigh, priceLow, timestamp
Djed->>Djed: compute scPrice, enforce TX_LIMIT, compute amounts
Djed->>Treasury: transfer(treasuryFee)
Djed->>Stable: mint(receiver, amountSc)
Djed-->>User: emit BoughtStableCoins
deactivate Djed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/DjedTefnut.sol (6)
2-2: Consider locking the Solidity version.Using a floating pragma (
^0.8.0) allows compilation with any 0.8.x version, which may introduce inconsistencies across deployments or unexpected behavior from compiler changes. For production contracts handling financial assets, lock to a specific tested version (e.g.,0.8.20).-pragma solidity ^0.8.0; +pragma solidity 0.8.20;
79-81: Equity underflow will revert the transaction.If liabilities exceed reserves (
R < L), this calculation will underflow and revert due to Solidity 0.8.x's built-in overflow protection. While this is the desired behavior (preventing under-collateralized operations), it could benefit from an explicit check with a descriptive error message for better debugging.function E(uint256 _scPrice, uint256 currentPaymentAmount) internal view returns (uint256) { + uint256 reserves = R(currentPaymentAmount); + uint256 liabilities = L(_scPrice); + require(reserves >= liabilities, "Insufficient reserves to cover liabilities"); - return R(currentPaymentAmount) - L(_scPrice); + return reserves - liabilities; }
204-207: ETH transfer implementation is safe.The use of low-level
callwith success check is the correct pattern for Solidity 0.8.x. Consider including the receiver address in the error message for better debugging.function transferEth(address receiver, uint256 amount) internal { (bool success,) = payable(receiver).call{value: amount}(""); - require(success, "Transfer failed."); + require(success, string(abi.encodePacked("Transfer failed to ", receiver))); }
96-146: Consider adding oracle manipulation protection.Each trading function calls
oracle.updateOracleValues()once, which could be exploited by an attacker who can manipulate the oracle in a single block (e.g., through flash loans or MEV). Consider:
- Implementing a time-weighted average price (TWAP) mechanism
- Requiring multiple oracle updates across different blocks
- Adding circuit breakers for extreme price movements
9-208: Architectural observation: Fully permissionless design.This contract has no access controls, pause mechanism, or upgrade path. While this may be an intentional design choice for trustlessness, be aware that:
- Once deployed, parameters cannot be adjusted even if market conditions change
- No emergency stop if critical bugs are discovered
- No ability to upgrade the oracle or treasury addresses
Verify this aligns with the project's governance and risk management strategy.
121-132: Add slippage protection for price volatility.This function lacks slippage protection (minimum RC tokens expected). Users are exposed to unfavorable price movements between transaction submission and confirmation.
Add slippage protection:
-function buyReserveCoins(address receiver, uint256 feeUi, address ui) external payable nonReentrant { +function buyReserveCoins(address receiver, uint256 feeUi, address ui, uint256 minAmountRc) external payable nonReentrant { oracle.updateOracleValues(); uint256 scP = scMinPrice(msg.value); uint256 rcBp = rcBuyingPrice(scP, msg.value); uint256 amountBc = deductFees(msg.value, feeUi, ui); require(amountBc <= (TX_LIMIT * scP) / SC_DECIMAL_SCALING_FACTOR || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "buyRC: tx limit exceeded"); uint256 amountRc = (amountBc * RC_DECIMAL_SCALING_FACTOR) / rcBp; + require(amountRc >= minAmountRc, "buyRC: slippage exceeded"); require(amountRc > 0, "buyRC: receiving zero RCs");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DjedTefnut.sol(1 hunks)
🔇 Additional comments (5)
src/DjedTefnut.sol (5)
4-7: LGTM!The imports are appropriate and necessary for the contract's functionality.
9-29: State variable organization looks good.The use of
immutablefor configuration parameters is appropriate and gas-efficient. The explicit initialization oftreasuryRevenueto 0 is redundant but provides clarity.
31-34: LGTM!Events are well-structured with appropriate indexed parameters for efficient filtering and querying.
167-182: LGTM! Stable coin pricing logic is sound.The
scPricefunction correctly implements the minimum of target price and fully-backed price, ensuring stable coins can't trade above their collateralization level. Division by zero is properly handled.
67-69: Clarify the semantic meaning of theR()function parameter.The
R()function correctly subtractscurrentPaymentAmountfromaddress(this).balance. During buy transactions, it's called withmsg.value, returning the reserve before the incoming payment is counted (sincemsg.valueis already in the balance). During sell transactions, it's called with0.However, the parameter name
currentPaymentAmountis semantically misleading—it suggests the amount to be added, not subtracted. The parameter actually represents "the amount to exclude from the current balance to compute the reserve at a prior state." Consider renaming toincomingPaymentAmountorpaymentToExcludeto clarify that this is the amount already included inaddress(this).balancethat should be subtracted out.
src/DjedTefnut.sol
Outdated
| // Treasury Parameters: | ||
| address public immutable TREASURY; // address of the treasury | ||
| uint256 public immutable TREASURY_FEE; // fixed treasury fee (no decay) | ||
| uint256 public treasuryRevenue = 0; // holds how much has already been paid to the treasury // Mutable state variable |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any functions that read or modify treasuryRevenue
rg -n 'treasuryRevenue' --type sol -C3Repository: DjedAlliance/Djed-Solidity
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for treasuryRevenue without type filter
rg -n 'treasuryRevenue' -C3Repository: DjedAlliance/Djed-Solidity
Length of output: 16335
🏁 Script executed:
#!/bin/bash
# Check if treasuryRevenue is read anywhere in DjedTefnut.sol
# (aside from the declaration and increment we already found)
cat -n src/DjedTefnut.sol | grep -E '(treasuryRevenue|treasuryFee)'Repository: DjedAlliance/Djed-Solidity
Length of output: 855
Add clarifying comment explaining treasuryRevenue is for external monitoring only.
The treasuryRevenue variable accumulates treasury fees but is never used in contract logic (unlike Djed.sol and DjedShu.sol where it drives dynamic fee calculations). Since it has no functional purpose beyond tracking, the existing inline comment should clarify that this variable is public for off-chain monitoring only and will never be reset.
🤖 Prompt for AI Agents
In src/DjedTefnut.sol around line 17, the inline comment for uint256 public
treasuryRevenue currently notes it holds amounts paid to the treasury but lacks
clarity about its purpose; update the comment on that line to state explicitly
that treasuryRevenue is a read-only accumulator exposed publicly for off-chain
monitoring/analytics only, is not used in contract logic or fee calculations,
and will never be reset so consumers should treat it as a cumulative metric.
| function buyStableCoins(address receiver, uint256 feeUi, address ui) external payable nonReentrant { | ||
| oracle.updateOracleValues(); | ||
| uint256 scP = scMaxPrice(msg.value); | ||
| uint256 amountBc = deductFees(msg.value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | ||
| uint256 amountSc = (amountBc * SC_DECIMAL_SCALING_FACTOR) / scP; | ||
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "buySC: tx limit exceeded"); | ||
| require(amountSc > 0, "buySC: receiving zero SCs"); | ||
| stableCoin.mint(receiver, amountSc); | ||
| // Reserve ratio check removed in Tefnut | ||
| emit BoughtStableCoins(msg.sender, receiver, amountSc, msg.value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add slippage protection for user safety.
The buyStableCoins function lacks slippage protection—users cannot specify a minimum amount of stable coins they're willing to accept. If the oracle price changes between transaction submission and execution, users could receive significantly fewer coins than expected.
-function buyStableCoins(address receiver, uint256 feeUi, address ui) external payable nonReentrant {
+function buyStableCoins(address receiver, uint256 feeUi, address ui, uint256 minAmountSc) external payable nonReentrant {
oracle.updateOracleValues();
uint256 scP = scMaxPrice(msg.value);
uint256 amountBc = deductFees(msg.value, feeUi, ui);
uint256 amountSc = (amountBc * SC_DECIMAL_SCALING_FACTOR) / scP;
+ require(amountSc >= minAmountSc, "buySC: slippage exceeded");
require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "buySC: tx limit exceeded");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function buyStableCoins(address receiver, uint256 feeUi, address ui) external payable nonReentrant { | |
| oracle.updateOracleValues(); | |
| uint256 scP = scMaxPrice(msg.value); | |
| uint256 amountBc = deductFees(msg.value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | |
| uint256 amountSc = (amountBc * SC_DECIMAL_SCALING_FACTOR) / scP; | |
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "buySC: tx limit exceeded"); | |
| require(amountSc > 0, "buySC: receiving zero SCs"); | |
| stableCoin.mint(receiver, amountSc); | |
| // Reserve ratio check removed in Tefnut | |
| emit BoughtStableCoins(msg.sender, receiver, amountSc, msg.value); | |
| } | |
| function buyStableCoins(address receiver, uint256 feeUi, address ui, uint256 minAmountSc) external payable nonReentrant { | |
| oracle.updateOracleValues(); | |
| uint256 scP = scMaxPrice(msg.value); | |
| uint256 amountBc = deductFees(msg.value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | |
| uint256 amountSc = (amountBc * SC_DECIMAL_SCALING_FACTOR) / scP; | |
| require(amountSc >= minAmountSc, "buySC: slippage exceeded"); | |
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "buySC: tx limit exceeded"); | |
| require(amountSc > 0, "buySC: receiving zero SCs"); | |
| stableCoin.mint(receiver, amountSc); | |
| // Reserve ratio check removed in Tefnut | |
| emit BoughtStableCoins(msg.sender, receiver, amountSc, msg.value); | |
| } |
🤖 Prompt for AI Agents
In src/DjedTefnut.sol around lines 96 to 106, the buyStableCoins function lacks
slippage protection; add a new uint256 parameter minAmountSc to the function
signature, compute amountSc as before, then require(amountSc >= minAmountSc,
"buySC: slippage exceeded") before the TX_LIMIT and zero checks (or combine
logically), and only mint/emit after that check; update any internal calls/tests
to pass the new parameter and document the behavior so users can specify a
minimum acceptable SC amount to protect against oracle price movement.
| function sellStableCoins(uint256 amountSc, address receiver, uint256 feeUi, address ui) external nonReentrant { | ||
| oracle.updateOracleValues(); | ||
| require(stableCoin.balanceOf(msg.sender) >= amountSc, "sellSC: insufficient SC balance"); | ||
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellSC: tx limit exceeded"); | ||
| uint256 scP = scMinPrice(0); | ||
| uint256 value = (amountSc * scP) / SC_DECIMAL_SCALING_FACTOR; | ||
| uint256 amountBc = deductFees(value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | ||
| require(amountBc > 0, "sellSC: receiving zero BCs"); | ||
| stableCoin.burn(msg.sender, amountSc); | ||
| transferEth(receiver, amountBc); | ||
| emit SoldStableCoins(msg.sender, receiver, amountSc, amountBc); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add slippage protection and consider transfer failure handling.
Similar to buyStableCoins, this function lacks slippage protection. Additionally, the ETH transfer on line 117 could fail if receiver is a contract that reverts on ETH receipt, causing the entire transaction (including the burn) to revert and potentially locking user funds if they retry.
Add a minimum received amount parameter:
-function sellStableCoins(uint256 amountSc, address receiver, uint256 feeUi, address ui) external nonReentrant {
+function sellStableCoins(uint256 amountSc, address receiver, uint256 feeUi, address ui, uint256 minAmountBc) external nonReentrant {
oracle.updateOracleValues();
require(stableCoin.balanceOf(msg.sender) >= amountSc, "sellSC: insufficient SC balance");
require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellSC: tx limit exceeded");
uint256 scP = scMinPrice(0);
uint256 value = (amountSc * scP) / SC_DECIMAL_SCALING_FACTOR;
uint256 amountBc = deductFees(value, feeUi, ui);
+ require(amountBc >= minAmountBc, "sellSC: slippage exceeded");
require(amountBc > 0, "sellSC: receiving zero BCs");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function sellStableCoins(uint256 amountSc, address receiver, uint256 feeUi, address ui) external nonReentrant { | |
| oracle.updateOracleValues(); | |
| require(stableCoin.balanceOf(msg.sender) >= amountSc, "sellSC: insufficient SC balance"); | |
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellSC: tx limit exceeded"); | |
| uint256 scP = scMinPrice(0); | |
| uint256 value = (amountSc * scP) / SC_DECIMAL_SCALING_FACTOR; | |
| uint256 amountBc = deductFees(value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | |
| require(amountBc > 0, "sellSC: receiving zero BCs"); | |
| stableCoin.burn(msg.sender, amountSc); | |
| transferEth(receiver, amountBc); | |
| emit SoldStableCoins(msg.sender, receiver, amountSc, amountBc); | |
| } | |
| function sellStableCoins(uint256 amountSc, address receiver, uint256 feeUi, address ui, uint256 minAmountBc) external nonReentrant { | |
| oracle.updateOracleValues(); | |
| require(stableCoin.balanceOf(msg.sender) >= amountSc, "sellSC: insufficient SC balance"); | |
| require(amountSc <= TX_LIMIT || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellSC: tx limit exceeded"); | |
| uint256 scP = scMinPrice(0); | |
| uint256 value = (amountSc * scP) / SC_DECIMAL_SCALING_FACTOR; | |
| uint256 amountBc = deductFees(value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | |
| require(amountBc >= minAmountBc, "sellSC: slippage exceeded"); | |
| require(amountBc > 0, "sellSC: receiving zero BCs"); | |
| stableCoin.burn(msg.sender, amountSc); | |
| transferEth(receiver, amountBc); | |
| emit SoldStableCoins(msg.sender, receiver, amountSc, amountBc); | |
| } |
| function sellReserveCoins(uint256 amountRc, address receiver, uint256 feeUi, address ui) external nonReentrant { | ||
| oracle.updateOracleValues(); | ||
| require(reserveCoin.balanceOf(msg.sender) >= amountRc, "sellRC: insufficient RC balance"); | ||
| uint256 scP = scMaxPrice(0); | ||
| uint256 value = (amountRc * rcTargetPrice(scP, 0)) / RC_DECIMAL_SCALING_FACTOR; | ||
| require(value <= (TX_LIMIT * scP) / SC_DECIMAL_SCALING_FACTOR || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellRC: tx limit exceeded"); | ||
| uint256 amountBc = deductFees(value, feeUi, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | ||
| require(amountBc > 0, "sellRC: receiving zero BCs"); | ||
| reserveCoin.burn(msg.sender, amountRc); | ||
| transferEth(receiver, amountBc); | ||
| // Reserve ratio check removed in Tefnut | ||
| emit SoldReserveCoins(msg.sender, receiver, amountRc, amountBc); | ||
| } |
There was a problem hiding this comment.
Add slippage protection and fix TX_LIMIT inconsistency.
The TX_LIMIT check on line 139 uses value (before fees), while buyReserveCoins line 126 checks amountBc (after fees). This inconsistency could allow bypass of limits or incorrect enforcement. Additionally, slippage protection is missing.
-function sellReserveCoins(uint256 amountRc, address receiver, uint256 feeUi, address ui) external nonReentrant {
+function sellReserveCoins(uint256 amountRc, address receiver, uint256 feeUi, address ui, uint256 minAmountBc) external nonReentrant {
oracle.updateOracleValues();
require(reserveCoin.balanceOf(msg.sender) >= amountRc, "sellRC: insufficient RC balance");
uint256 scP = scMaxPrice(0);
uint256 value = (amountRc * rcTargetPrice(scP, 0)) / RC_DECIMAL_SCALING_FACTOR;
- require(value <= (TX_LIMIT * scP) / SC_DECIMAL_SCALING_FACTOR || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellRC: tx limit exceeded");
uint256 amountBc = deductFees(value, feeUi, ui);
+ require(amountBc <= (TX_LIMIT * scP) / SC_DECIMAL_SCALING_FACTOR || stableCoin.totalSupply() < THRESHOLD_SUPPLY_SC, "sellRC: tx limit exceeded");
+ require(amountBc >= minAmountBc, "sellRC: slippage exceeded");
require(amountBc > 0, "sellRC: receiving zero BCs");🤖 Prompt for AI Agents
In src/DjedTefnut.sol around lines 134-146, the TX_LIMIT is checked against
pre-fee `value` and slippage protection is missing; change the flow to compute
`amountBc` after calling deductFees, then enforce TX_LIMIT against `amountBc`
(matching buyReserveCoins behavior) and add a slippage-protection input (e.g.,
minAmountBc) to the function signature, requiring amountBc >= minAmountBc before
burning and transferring; also move the reserveCoin.burn to after these checks
and keep the emitted event intact. Ensure the buyReserveCoins path uses the same
post-fee TX_LIMIT logic for consistency.
| function deductFees(uint256 value, uint256 feeUi, address ui) internal returns (uint256) { | ||
| uint256 f = (value * FEE) / SCALING_FACTOR; | ||
| uint256 fUi = (value * feeUi) / SCALING_FACTOR; | ||
| uint256 fT = (value * TREASURY_FEE) / SCALING_FACTOR; // Fixed treasury fee (no decay) | ||
| treasuryRevenue += fT; | ||
| transferEth(TREASURY, fT); | ||
| transferEth(ui, fUi); | ||
| // transferEth(address(this), f); // this happens implicitly, and thus `f` is effectively transferred to the reserve. | ||
| return value - f - fUi - fT; // amountBc | ||
| } |
There was a problem hiding this comment.
Validate fees and protect against DoS via malicious UI address.
The function has several concerns:
-
DoS Risk: If
uiis a contract that reverts on ETH receipt, all trading would fail. Validateui != address(0)and consider wrapping transfers in try/catch. -
Fee Validation: No runtime check that
feeUi + FEE + TREASURY_FEE <= SCALING_FACTOR. If the sum exceeds 100%, line 160 would underflow and revert (in 0.8.x). Add validation in the constructor or here. -
Checks-Effects-Interactions: State change (line 156) occurs before external calls (lines 157-158). While protected by
nonReentrant, follow CEI pattern for defense in depth.
function deductFees(uint256 value, uint256 feeUi, address ui) internal returns (uint256) {
+ require(feeUi + FEE + TREASURY_FEE <= SCALING_FACTOR, "Total fees exceed 100%");
uint256 f = (value * FEE) / SCALING_FACTOR;
uint256 fUi = (value * feeUi) / SCALING_FACTOR;
uint256 fT = (value * TREASURY_FEE) / SCALING_FACTOR;
treasuryRevenue += fT;
- transferEth(TREASURY, fT);
- transferEth(ui, fUi);
+ if (fT > 0) {
+ transferEth(TREASURY, fT);
+ }
+ if (fUi > 0 && ui != address(0)) {
+ transferEth(ui, fUi);
+ }
// transferEth(address(this), f); // this happens implicitly, and thus `f` is effectively transferred to the reserve.
return value - f - fUi - fT; // amountBc
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function deductFees(uint256 value, uint256 feeUi, address ui) internal returns (uint256) { | |
| uint256 f = (value * FEE) / SCALING_FACTOR; | |
| uint256 fUi = (value * feeUi) / SCALING_FACTOR; | |
| uint256 fT = (value * TREASURY_FEE) / SCALING_FACTOR; // Fixed treasury fee (no decay) | |
| treasuryRevenue += fT; | |
| transferEth(TREASURY, fT); | |
| transferEth(ui, fUi); | |
| // transferEth(address(this), f); // this happens implicitly, and thus `f` is effectively transferred to the reserve. | |
| return value - f - fUi - fT; // amountBc | |
| } | |
| function deductFees(uint256 value, uint256 feeUi, address ui) internal returns (uint256) { | |
| require(feeUi + FEE + TREASURY_FEE <= SCALING_FACTOR, "Total fees exceed 100%"); | |
| uint256 f = (value * FEE) / SCALING_FACTOR; | |
| uint256 fUi = (value * feeUi) / SCALING_FACTOR; | |
| uint256 fT = (value * TREASURY_FEE) / SCALING_FACTOR; // Fixed treasury fee (no decay) | |
| treasuryRevenue += fT; | |
| if (fT > 0) { | |
| transferEth(TREASURY, fT); | |
| } | |
| if (fUi > 0 && ui != address(0)) { | |
| transferEth(ui, fUi); | |
| } | |
| // transferEth(address(this), f); // this happens implicitly, and thus `f` is effectively transferred to the reserve. | |
| return value - f - fUi - fT; // amountBc | |
| } |
🤖 Prompt for AI Agents
In src/DjedTefnut.sol around lines 152–161, validate and harden fee handling by
(1) enforcing ui != address(0) at the start of deductFees (or at call sites) to
prevent trivial DoS, (2) ensuring feeUi + FEE + TREASURY_FEE <= SCALING_FACTOR
(preferably validated once in the constructor and re-checked here defensively),
(3) compute all fee amounts first, update state (treasuryRevenue) before
performing external transfers to follow CEI, and (4) avoid DoS from a reverting
ui by using a low-level call for the ui transfer and handling failure (e.g., on
failure route fUi to TREASURY or record it in treasuryRevenue) so a malicious UI
contract cannot block the function. Ensure any transfer failures are handled
deterministically (not left to revert) and log or record failures where
appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/DjedTefnut.sol (4)
102-114: Add user slippage protection tobuyStableCoinsUsers can be sandwiched by oracle updates between signing and mining; consider adding a
minAmountSc(ormaxScPrice) parameter and revert if the computed mint is worse than expected.
116-127: Add user slippage protection tosellStableCoinsSame concern in reverse: users need
minAmountBc(orminPayoutWei) to avoid price movement delivering less ETH than expected.
142-154: TX_LIMIT check should be consistent with post-fee amount insellReserveCoinsHere TX limit is enforced on pre-fee
value, whilebuyReserveCoinsenforces on post-feeamountBc. Recommend enforcing TX_LIMIT using the same “net after fees” basis across buy/sell to avoid inconsistencies.
160-170:deductFees: UI transfer can DoS trading; alsoui != address(0)is restrictiveRequiring a nonzero
uiand hard-reverting ontransferEth(ui, ...)means a malicious/reverting UI contract (or a misconfigured UI address) can block trading. Consider allowingui == address(0)(treat as no-UI fee) and/or making UI fee transfer non-blocking (e.g., accrue to treasury or a withdrawable balance on failure).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DjedTefnut.sol(1 hunks)
🔇 Additional comments (5)
src/DjedTefnut.sol (5)
35-72: Constructor validation additions look solidGood early reverts for misconfiguration (
oracleAddress,treasury, scaling/fee bounds, min/initial price, thresholds). Also good to callacceptTermsOfService()only after state is set.
74-100:R/L/E/ratioare coherent; ratio “informational only” behavior is clear
R(currentPaymentAmount)correctly neutralizesmsg.valueinclusion for pricing in payable paths; returningtype(uint256).maxon zero liabilities is a reasonable sentinel.
129-140: RC buy path: sanity-check intended choice ofscMinPrice(msg.value)
buyReserveCoinsusesscMinPrice(msg.value)whilercBuyingPrice(...)is derived fromrcTargetPricewhich is very sensitive to_scPricevia liabilities/equity. Please confirm this is the intended economic choice (vs usingscMaxPrice).
193-212: Nice: explicit under-collateralization revert + zero-supply checkThe
require(R(...) >= L(...), "Under-collateralized: reserve < liability")makes the intended “RC ops disabled when insolvent” behavior much clearer than an implicit underflow.
4-6: The OpenZeppelin import path is correct. The repository pins OpenZeppelin Contracts v4.8.1 via git submodule (commit 0457042d93d9dfd760dbaa06a4d2f1216fdbe297), and@openzeppelin/contracts/security/ReentrancyGuard.solis the valid path for that version. (Note: This path changed in v5.0+, where ReentrancyGuard moved toutils/, so the pinned version matters—but it's correctly matched here.)
| // SPDX-License-Identifier: AEL | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; | ||
| import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | ||
| import {Coin} from "./Coin.sol"; | ||
| import {IOracleShu} from "./IOracleShu.sol"; | ||
|
|
There was a problem hiding this comment.
SPDX identifier AEL may be invalid for tooling/verification
If AEL isn’t a recognized SPDX license identifier in your pipeline/tooling, you’ll get warnings or verification friction. Consider switching to a standard SPDX id (or UNLICENSED) consistent with the repo’s licensing.
🤖 Prompt for AI Agents
In src/DjedTefnut.sol around lines 1 to 8, the SPDX license identifier at the
top is set to "AEL", which may not be a recognized SPDX identifier and can
trigger tooling/verification warnings; update the SPDX header to a valid SPDX
identifier used by the repo (for example "MIT", "GPL-3.0-or-later", etc.) or use
"UNLICENSED" if the code is not intended to be published, and ensure it exactly
matches the SPDX identifier casing and syntax (SPDX-License-Identifier: <ID>).
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AEL SPDX license identifier
💡 Result:
Do you mean a specific license abbreviated “AEL”? If so, what does AEL stand for (full license name or a link)? I can then look up the correct SPDX identifier.
🌐 Web query:
official SPDX license list identifiers 2025
💡 Result:
The official SPDX license identifiers are published on the SPDX License List page and in the SPDX license-list-data repository (machine-readable JSON/XML). Use the licenses page for the human-readable table and the GitHub repo (licenses.json / exceptions.json) for authoritative identifiers you can consume programmatically. Current references:
- SPDX License List (web page, version 3.27.0, 2025). [1]
- SPDX license-list-data (GitHub) — machine-readable files (json/xml) containing all identifiers. [2]
If you want the full JSON file of identifiers, download licenses.json from the license-list-data repo. [2]
Sources
[1] SPDX: SPDX License List — https://spdx.org/licenses/
[2] spdx/license-list-data — https://github.com/spdx/license-list-data
🏁 Script executed:
#!/bin/bash
# Fetch the official SPDX license list and check if "AEL" is a valid identifier
curl -s https://raw.githubusercontent.com/spdx/license-list-data/main/json/licenses.json | jq -r '.licenses[] | .licenseId' | grep -i "^AEL$"
Length of output: 0
@negimox, I've checked the official SPDX license list (version 3.27.0), and "AEL" is not a recognized standard SPDX license identifier. So CodeRabbit's warning is technically accurate from a standards perspective.
However, if "AEL" is a custom or proprietary license identifier specific to your project (such as "All Emcompassing License" or similar), then using it may be intentional. Could you clarify:
- What does "AEL" stand for in your project's context?
- Is this a custom license identifier you're using intentionally?
If it's a custom identifier and intentional, you can safely ignore the warning. If you meant to use a standard SPDX identifier, I can help you find the appropriate one. Alternatively, if the code is proprietary/closed-source, you might want to use UNLICENSED instead.
|
Hey, @yogesh0509 could you please if the above code rabbit actionable items? I believe they are incorrect could you provide your inputs? As iam utilizing the correct AEL license |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/DjedTefnut.t.sol (2)
415-426: Weak assertion may mask test failures.The assertion
scPriceAfter != scPriceBefore || djed.stableCoin().totalSupply() > 0will always pass whentotalSupply() > 0, regardless of whether the oracle price change actually affected the SC price. SincetotalSupply()is 0 in this test (no SC bought), it should work, but the compound condition is confusing.Consider a more direct assertion:
- assertTrue(scPriceAfter != scPriceBefore || djed.stableCoin().totalSupply() > 0, - "Price should reflect oracle change"); + assertTrue(scPriceAfter != scPriceBefore, "SC price should change when oracle price changes");
374-392: Consider adding negative test for TX_LIMIT enforcement.This test verifies small transactions succeed under TX_LIMIT, but doesn't verify that exceeding TX_LIMIT actually reverts. Adding a test that attempts to exceed the limit would strengthen coverage of this feature.
Would you like me to generate a test case that verifies TX_LIMIT enforcement by attempting a transaction that exceeds the limit?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/DeploymentParametersTefnut.sol(1 hunks)scripts/deployDjedTefnutContract.s.sol(1 hunks)src/mock/MockShuOracleTefnut.sol(1 hunks)src/test/DjedTefnut.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: negimox
Repo: DjedAlliance/Djed-Solidity PR: 41
File: src/DjedTefnut.sol:214-217
Timestamp: 2025-12-13T17:46:15.360Z
Learning: In Djed protocol implementations (e.g., DjedTefnut.sol), push payments are used intentionally to maintain consistency with the original Djed protocol architecture, rather than pull payment patterns.
🔇 Additional comments (5)
src/mock/MockShuOracleTefnut.sol (1)
1-82: LGTM! Clean mock oracle implementation.The mock oracle correctly implements the
IOracleShuinterface with appropriate test helpers. The validation insetPricesanddecreasePriceproperly prevents invalid states, and Solidity 0.8.x provides built-in overflow protection forincreasePrice.scripts/deployDjedTefnutContract.s.sol (1)
26-87: Well-structured deployment script with dynamic oracle handling.The script cleanly handles both local/test deployments (with mock oracle) and production deployments (with provided oracle). The comprehensive logging will aid debugging. Good use of the parameters abstraction via inheritance.
src/test/DjedTefnut.t.sol (1)
55-79: Solid test setup with good coverage of Tefnut-specific behavior.The test suite comprehensively covers the key Tefnut differentiator (no reserve ratio restrictions) with dedicated tests in lines 235-297. The setup correctly initializes the mock oracle and DjedTefnut with matching parameters.
scripts/DeploymentParametersTefnut.sol (2)
29-29: Sepolia oracle address is a placeholder.
SEPOLIA_SHU_ORACLE_ADDRESS = address(0)means Sepolia deployments will also deploy aMockShuOracleTefnut. Ensure this is intentional for testnet, or update with the actual oracle address before production deployment.
40-68: Clean configuration pattern with good chain-specific separation.The parameter struct and chain-based configuration approach is well-organized. The fallback to local/Anvil configuration for unknown chains is a sensible default for development.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/DeploymentParametersTefnut.sol (1)
66-66: Incorrect comment: limit is 200M SC, not 200 SC.The value
200e6equals 200 million SC, not 200 SC. If the intent is 200 SC, the value should be200.Apply this diff if the intent is 200 SC:
- txLimit: 200e6 // 200 SC + txLimit: 200 // 200 SCOr update the comment to reflect the actual value:
- txLimit: 200e6 // 200 SC + txLimit: 200e6 // 200M SC
🧹 Nitpick comments (1)
scripts/DeploymentParametersTefnut.sol (1)
54-68: Unrecognized chains default to local configuration.The else branch applies local/test configuration to any chain ID that isn't Sepolia. This could be risky if the contract is accidentally deployed to an unrecognized production network (e.g., mainnet).
Consider adding explicit handling for known production chains or reverting on unrecognized chains:
} else { + // Explicitly revert for mainnet to prevent accidental deployment with test config + require(chainId != 1, "Mainnet deployment not configured"); + // Local/Anvil Default Configuration // oracleAddress = address(0) signals that a mock oracle should be deployed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/DeploymentParametersTefnut.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: negimox
Repo: DjedAlliance/Djed-Solidity PR: 41
File: src/DjedTefnut.sol:214-217
Timestamp: 2025-12-13T17:46:15.360Z
Learning: In Djed protocol implementations (e.g., DjedTefnut.sol), push payments are used intentionally to maintain consistency with the original Djed protocol architecture, rather than pull payment patterns.
🔇 Additional comments (3)
scripts/DeploymentParametersTefnut.sol (3)
50-51: Previous issue resolved: price comments are now correct.The comments for
rcMinPriceandrcInitialPricehave been updated from the previous review and now correctly reflect "1 ETH per RC" and "100 ETH per RC" respectively.
64-65: Previous issue resolved: price comments are now correct.The comments for
rcMinPriceandrcInitialPricein the local configuration have also been updated correctly.
76-84: LGTM!The
getNetworkNamefunction correctly maps chain IDs to human-readable network names with a sensible default for unknown networks.
| uint256 constant ANVIL_CHAIN_ID = 31337; | ||
|
|
||
| // Known oracle addresses | ||
| address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // Placeholder - replace with actual address when deployed |
There was a problem hiding this comment.
Sepolia will deploy with mock oracle instead of real oracle.
The SEPOLIA_SHU_ORACLE_ADDRESS is set to address(0) as a placeholder. However, the deployment script interprets address(0) as a signal to deploy MockShuOracleTefnut. This means Sepolia deployments will use a mock oracle rather than a production oracle, which may not be intended for testnet deployments.
Update the placeholder comment to clarify this behavior, or replace with the actual Sepolia oracle address when available:
- address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // Placeholder - replace with actual address when deployed
+ address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // IMPORTANT: address(0) triggers mock oracle deployment. Replace with actual Sepolia oracle address.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // Placeholder - replace with actual address when deployed | |
| address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // IMPORTANT: address(0) triggers mock oracle deployment. Replace with actual Sepolia oracle address. |
🤖 Prompt for AI Agents
In scripts/DeploymentParametersTefnut.sol around line 29, the
SEPOLIA_SHU_ORACLE_ADDRESS is set to address(0) which your deployment script
treats as a signal to deploy MockShuOracleTefnut; update the line comment to
explicitly state that address(0) will cause the mock oracle to be deployed on
Sepolia or, if you intend to use a real oracle, replace address(0) with the
actual Sepolia oracle address when available so the deployment uses the
production oracle instead of the mock.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/DeploymentParametersTefnut.sol (1)
29-29: Past issue not fully addressed: Clarify that address(0) triggers mock oracle deployment.While previous comments were marked as addressed, the Sepolia oracle address comment still doesn't clearly state that
address(0)will cause the deployment script to use a mock oracle. Line 56 in the local config explicitly documents this behavior with "Signal to deploy mock", but the Sepolia constant lacks this clarity.Apply this diff for consistency with line 56's documentation style:
- address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // Placeholder - replace with actual address when deployed + address constant SEPOLIA_SHU_ORACLE_ADDRESS = address(0); // IMPORTANT: address(0) triggers mock oracle deployment. Replace with actual Sepolia oracle address when available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/DeploymentParametersTefnut.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: negimox
Repo: DjedAlliance/Djed-Solidity PR: 41
File: src/DjedTefnut.sol:214-217
Timestamp: 2025-12-13T17:46:15.360Z
Learning: In Djed protocol implementations (e.g., DjedTefnut.sol), push payments are used intentionally to maintain consistency with the original Djed protocol architecture, rather than pull payment patterns.
| thresholdSupplySc: 1e6, // 1M SC | ||
| rcMinPrice: 1e18, // 1 ETH per RC | ||
| rcInitialPrice: 1e20, // 100 ETH per RC | ||
| txLimit: 200e6 // 200 SC |
There was a problem hiding this comment.
Incorrect comment: transaction limit is 200M SC, not 200 SC.
The value 200e6 equals 200,000,000 (200 million SC), not 200 SC. The comment is off by a factor of 1,000,000.
Apply this diff to correct the comment:
- txLimit: 200e6 // 200 SC
+ txLimit: 200e6 // 200M SC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| txLimit: 200e6 // 200 SC | |
| txLimit: 200e6 // 200M SC |
🤖 Prompt for AI Agents
In scripts/DeploymentParametersTefnut.sol around line 66, the inline comment for
txLimit incorrectly states "200 SC" while the value 200e6 equals 200,000,000
(200 million) SC; update the comment to reflect the correct amount such as
"200,000,000 SC (200M SC)" so it accurately documents the numeric value.
1 participant
Implemented Djed Tefnut smart contracts with ETH backing. For issue #37
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.