Make stablecoin and reservecoin names configurable #42
Conversation
WalkthroughThe PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Djed.sol (1)
47-69: Contract deployment creates a vulnerable window.The constructor no longer initializes the coins, leaving the contract in an incomplete state immediately after deployment. Between deployment and calling
initializeCoins, the contract is:
- Unusable for its intended purpose
- Vulnerable to frontrunning (see critical issue on lines 72-92)
Ensure deployment scripts atomically call
initializeCoinsin the same transaction or block to minimize this window.
🧹 Nitpick comments (1)
src/Djed.sol (1)
14-19: Consider exposing the initialization status.The
coinsInitializedflag is private, preventing external verification of whether the contract is ready to use. Consider either making it public or adding a getter function so users and integrations can check the contract's initialization state before interacting with it.Apply this diff to make the initialization status public:
- bool private coinsInitialized; + bool public coinsInitialized;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Djed.sol(3 hunks)
🔇 Additional comments (2)
src/Djed.sol (2)
100-100: Incomplete diff prevents full review.The comment indicates the rest of the contract remains unchanged, but without seeing the complete file, it's impossible to verify:
- Whether the
coinsReadymodifier has been added to all necessary functions- Whether there are any other references to coin contracts that need updating
- Whether the changes integrate correctly with existing functionality
Consider providing the full contract file for a comprehensive review.
95-98: This review comment is based on an incorrect assumption. ThecoinsReadymodifier in src/Djed.sol (lines 95-98) is correctly defined, but the file is incomplete—it ends at line 101 with only the modifier definition and no functions that would use it.The functions mentioned in the review (
buyStableCoins,sellStableCoins,buyReserveCoins,sellReserveCoins,sellBothCoins) do not exist in src/Djed.sol. They are implemented in a separate contract, src/DjedShu.sol, which does not inherit from Djed and does not use thecoinsReadypattern. DjedShu initializes coins in its constructor instead of via an externalinitializeCoins()function.src/Djed.sol appears to be an incomplete base contract stub. If it is intended to be completed with trading functions, the
coinsReadymodifier should be applied to all functions that accessstableCoin,reserveCoin,scDecimalScalingFactor, orrcDecimalScalingFactor. However, as currently committed, the contract contains no such functions.Likely an incorrect or invalid review comment.
| uint256 public scDecimalScalingFactor; | ||
| uint256 public rcDecimalScalingFactor; |
There was a problem hiding this comment.
Scaling factors are no longer immutable, increasing gas costs.
Changing scDecimalScalingFactor and rcDecimalScalingFactor from immutable to mutable has implications:
- Gas cost: Every read now requires an SLOAD (~2100 gas cold, 100 gas warm) instead of direct value substitution
- Safety: These values can theoretically be modified if there's any bug in future code changes
While this change is necessary for the two-step initialization pattern, consider whether the flexibility gained outweighs the increased operational costs, especially if these scaling factors are read frequently in trading functions.
| function initializeCoins( | ||
| string memory _stableCoinName, | ||
| string memory _stableCoinSymbol, | ||
| string memory _reserveCoinName, | ||
| string memory _reserveCoinSymbol | ||
| ) external { | ||
| require(!coinsInitialized, "Coins already initialized"); | ||
|
|
||
| function ratio() external view returns (uint256) { | ||
| return scalingFactor * R(0) / L(scPrice(0)); | ||
| } | ||
| stableCoinName = _stableCoinName; | ||
| stableCoinSymbol = _stableCoinSymbol; | ||
| reserveCoinName = _reserveCoinName; | ||
| reserveCoinSymbol = _reserveCoinSymbol; | ||
|
|
||
| // # Public Trading Functions: | ||
|
|
||
| function buyStableCoins(address receiver, uint256 feeUI, address ui) external payable nonReentrant { | ||
| uint256 scP = scPrice(msg.value); | ||
| uint256 amountBC = deductFees(msg.value, feeUI, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | ||
| uint256 amountSC = (amountBC * scDecimalScalingFactor) / scP; | ||
| require(amountSC <= txLimit || stableCoin.totalSupply() < thresholdSupplySC, "buySC: tx limit exceeded"); | ||
| require(amountSC > 0, "buySC: receiving zero SCs"); | ||
| stableCoin.mint(receiver, amountSC); | ||
| require(isRatioAboveMin(scPrice(0)), "buySC: ratio below min"); | ||
| emit BoughtStableCoins(msg.sender, receiver, amountSC, msg.value); | ||
| } | ||
| stableCoin = new Coin(_stableCoinName, _stableCoinSymbol); | ||
| reserveCoin = new Coin(_reserveCoinName, _reserveCoinSymbol); | ||
|
|
||
| function sellStableCoins(uint256 amountSC, address receiver, uint256 feeUI, address ui) external nonReentrant { | ||
| require(stableCoin.balanceOf(msg.sender) >= amountSC, "sellSC: insufficient SC balance"); | ||
| require(amountSC <= txLimit || stableCoin.totalSupply() < thresholdSupplySC, "sellSC: tx limit exceeded"); | ||
| uint256 scP = scPrice(0); | ||
| uint256 value = (amountSC * scP) / scDecimalScalingFactor; | ||
| 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); | ||
| transfer(receiver, amountBC); | ||
| emit SoldStableCoins(msg.sender, receiver, amountSC, amountBC); | ||
| } | ||
| scDecimalScalingFactor = 10 ** stableCoin.decimals(); | ||
| rcDecimalScalingFactor = 10 ** reserveCoin.decimals(); | ||
|
|
||
| function buyReserveCoins(address receiver, uint256 feeUI, address ui) external payable nonReentrant { | ||
| uint256 scP = scPrice(msg.value); | ||
| uint256 rcBP = rcBuyingPrice(scP, msg.value); | ||
| uint256 amountBC = deductFees(msg.value, feeUI, ui); // side-effect: increases `treasuryRevenue` and pays UI and treasury | ||
| require(amountBC <= (txLimit * scP) / scDecimalScalingFactor || stableCoin.totalSupply() < thresholdSupplySC, "buyRC: tx limit exceeded"); | ||
| uint256 amountRC = (amountBC * rcDecimalScalingFactor) / rcBP; | ||
| require(amountRC > 0, "buyRC: receiving zero RCs"); | ||
| reserveCoin.mint(receiver, amountRC); | ||
| require(isRatioBelowMax(scPrice(0)) || stableCoin.totalSupply() < thresholdSupplySC, "buyRC: ratio above max"); | ||
| emit BoughtReserveCoins(msg.sender, receiver, amountRC, msg.value); | ||
| coinsInitialized = true; | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing access control allows anyone to initialize coins.
The initializeCoins function has no access control, allowing any external caller to initialize the contract and set token names/symbols. This creates a severe frontrunning vulnerability:
- An attacker monitoring the mempool can call
initializeCoinsbefore the legitimate deployer - The attacker controls coin names/symbols, potentially setting misleading values like "USDT"/"USDC" to trick users
- Once initialized, the configuration cannot be changed due to the
coinsInitializedguard
Additionally:
- No validation on input strings (empty, too long, malicious characters)
- No event emission to track initialization
- No way to verify the initializer's identity
Apply this diff to add access control:
+ address public immutable deployer;
+
+ constructor(...) {
+ deployer = msg.sender;
// ... existing constructor code
}
function initializeCoins(
string memory _stableCoinName,
string memory _stableCoinSymbol,
string memory _reserveCoinName,
string memory _reserveCoinSymbol
) external {
+ require(msg.sender == deployer, "Only deployer can initialize");
require(!coinsInitialized, "Coins already initialized");
+ require(bytes(_stableCoinName).length > 0 && bytes(_stableCoinName).length <= 32, "Invalid stablecoin name");
+ require(bytes(_stableCoinSymbol).length > 0 && bytes(_stableCoinSymbol).length <= 8, "Invalid stablecoin symbol");
+ require(bytes(_reserveCoinName).length > 0 && bytes(_reserveCoinName).length <= 32, "Invalid reservecoin name");
+ require(bytes(_reserveCoinSymbol).length > 0 && bytes(_reserveCoinSymbol).length <= 8, "Invalid reservecoin symbol");
stableCoinName = _stableCoinName;
stableCoinSymbol = _stableCoinSymbol;
reserveCoinName = _reserveCoinName;
reserveCoinSymbol = _reserveCoinSymbol;
stableCoin = new Coin(_stableCoinName, _stableCoinSymbol);
reserveCoin = new Coin(_reserveCoinName, _reserveCoinSymbol);
scDecimalScalingFactor = 10 ** stableCoin.decimals();
rcDecimalScalingFactor = 10 ** reserveCoin.decimals();
coinsInitialized = true;
+ emit CoinsInitialized(_stableCoinName, _stableCoinSymbol, _reserveCoinName, _reserveCoinSymbol);
}Also add the event declaration:
event CoinsInitialized(string stableCoinName, string stableCoinSymbol, string reserveCoinName, string reserveCoinSymbol);🤖 Prompt for AI Agents
In src/Djed.sol around lines 72 to 92, the initializeCoins function lacks access
control and input validation; restrict it so only the contract owner (or a
designated initializer) can call it by adding an owner state variable (set at
construction) and require(msg.sender == owner) or use an onlyOwner modifier,
keep the existing coinsInitialized guard, validate input strings (non-empty and
reasonable max length) before using them, emit a
CoinsInitialized(string,string,string,string) event after successful
initialization, and (optionally) store the initializer address for auditability;
ensure decimals are read after coin deployment and all state changes occur
before emitting the event.
1 participant
Fixes #18
This PR removes hardcoded StableCoin and ReserveCoin names/symbols and
adds a one-time initialization function to configure token metadata.
This avoids increasing the constructor argument count, as suggested
in the issue.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.