Foundry DeFi Stablecoin CodeHawks Audit Contest
Cyfrin
DeFiFoundry
15,000 USDC
View results
Previous Next
Submission Details
Severity: gas
Valid

`Ownable` and `ERC20Burneable` implementations arent necesary in `DecentralizedStableCoin`

LaScaloneta

Summary

An optimization issue has been identified in the DecentralizedStableCoin smart contract. It involves replacing the OpenZeppelin's Ownable contract with a simple owner state variable and removing ERC20Burnable. This will improve gas efficiency and simplify the contract.

Vulnerability Details

While this is not a security vulnerability per se, the current implementation is inefficient and could be improved.

  1. The contract uses the Ownable contract from OpenZeppelin. The Ownable contract is designed to provide a general ownership solution, which can be overkill in many situations. In this case, the DecentralizedStableCoin is always owned by DSCEngine, so a simpler ownership pattern can be used.

  2. The DecentralizedStableCoin contract extends ERC20Burnable, which provides an additional burnFrom method that allows third parties to burn tokens from a user's account (provided they have an allowance). This functionality is not required in the current context.

Impact

This proposed optimization would result in a gas reduction of approximately 0.264%. While this might seem insignificant, it's important to note that in high-volume systems, this could translate to substantial savings.

Moreover, simplifying the contract also makes it easier to understand and maintain, reducing the risk of bugs and vulnerabilities.

Benchmark results

testUserStillHasSomeEthAfterLiquidation() (gas: -1992 (-0.400%))
testLiquidationPayoutIsCorrect() (gas: -1992 (-0.411%))
testLiquidatorTakesOnUsersDebt() (gas: -1992 (-0.412%))
testUserHasNoMoreDebt() (gas: -1992 (-0.412%))
testCantLiquidateGoodHealthFactor() (gas: -2204 (-0.622%))
testHealthFactorCanGoBelowOne() (gas: -2102 (-0.725%))
testCanRedeemDepositedCollateral() (gas: -1893 (-0.752%))
testCanBurnDsc() (gas: -1893 (-0.798%))
testMustRedeemMoreThanZero() (gas: -1911 (-0.806%))
testProperlyReportsHealthFactor() (gas: -2102 (-1.000%))
testCanMintDsc() (gas: -2080 (-1.018%))
testCanMintWithDepositedCollateral() (gas: -2080 (-1.024%))
testRevertsIfRedeemAmountIsZero() (gas: -2102 (-1.044%))
testRevertsIfBurnAmountIsZero() (gas: -2102 (-1.045%))
testRevertsIfMintAmountIsZero() (gas: -2102 (-1.045%))
testCantBurnMoreThanYouHave() (gas: -2420 (-4.034%))
testMustBurnMoreThanZero() (gas: -2420 (-4.035%))
testCantMintToZeroAddress() (gas: -2230 (-17.922%))
testMustMintMoreThanZero() (gas: -2230 (-18.488%))
Overall gas change: -39821 (-0.264%)

Tools Used

forge snapshot --diff

Recommendations

It's recommended to replace the Ownable contract with a simple owner state variable. This will allow to remove the ERC20Burnable and simplifying the burn method.

diff --git a/src/DecentralizedStableCoin.sol b/src/DecentralizedStableCoin.sol
index d3652a5..58cd9b7 100644
--- a/src/DecentralizedStableCoin.sol
+++ b/src/DecentralizedStableCoin.sol
@@ -36,14 +36,19 @@ import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
* This is the contract meant to be governed by DSCEngine. This contract is just the ERC20 implementation of our stablecoin system.
*
*/
-contract DecentralizedStableCoin is ERC20Burnable, Ownable {
+contract DecentralizedStableCoin is ERC20 {
+ address private immutable i_owner;
+
error DecentralizedStableCoin__MustBeMoreThanZero();
error DecentralizedStableCoin__BurnAmountExceedsBalance();
error DecentralizedStableCoin__NotZeroAddress();
- constructor() ERC20("DecentralizedStableCoin", "DSC") {}
+ constructor(address _owner) ERC20("DecentralizedStableCoin", "DSC") {
+ i_owner = _owner;
+ }
- function burn(uint256 _amount) public override onlyOwner {
+ function burn(uint256 _amount) external {
+ require(i_owner == msg.sender, "Only owner can burn");
uint256 balance = balanceOf(msg.sender);
if (_amount <= 0) {
revert DecentralizedStableCoin__MustBeMoreThanZero();
@@ -51,10 +56,11 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
if (balance < _amount) {
revert DecentralizedStableCoin__BurnAmountExceedsBalance();
}
- super.burn(_amount);
+ _burn(msg.sender, _amount);
}
- function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
+ function mint(address _to, uint256 _amount) external returns (bool) {
+ require(i_owner == msg.sender, "Only owner can mint");
if (_to == address(0)) {
revert DecentralizedStableCoin__NotZeroAddress();
}
@@ -64,4 +70,8 @@ contract DecentralizedStableCoin is ERC20Burnable, Ownable {
_mint(_to, _amount);
return true;
}
+
+ function owner() external view returns (address) {
+ return i_owner;
+ }
}
​
diff --git a/test/unit/DecentralizedStablecoinTest.t.sol b/test/unit/DecentralizedStablecoinTest.t.sol
index e745c53..94c8844 100644
--- a/test/unit/DecentralizedStablecoinTest.t.sol
+++ b/test/unit/DecentralizedStablecoinTest.t.sol
@@ -10,7 +10,7 @@ contract DecentralizedStablecoinTest is StdCheats, Test {
DecentralizedStableCoin dsc;
function setUp() public {
- dsc = new DecentralizedStableCoin();
+ dsc = new DecentralizedStableCoin(address(this));
}
function testMustMintMoreThanZero() public {
​
diff --git a/script/DeployDSC.s.sol b/script/DeployDSC.s.sol
index 24192fb..a1b0aba 100644
--- a/script/DeployDSC.s.sol
+++ b/script/DeployDSC.s.sol
@@ -21,10 +21,14 @@ contract DeployDSC is Script {
priceFeedAddresses = [wethUsdPriceFeed, wbtcUsdPriceFeed];
vm.startBroadcast(deployerKey);
- DecentralizedStableCoin dsc = new DecentralizedStableCoin();
- DSCEngine engine = new DSCEngine(tokenAddresses, priceFeedAddresses, address(dsc));
- dsc.transferOwnership(address(engine));
+ address predictedEngine = 0xdBc6bEB03Bd0829E8f48A86D121d448c364D3983;
+
+ DecentralizedStableCoin dsc = new DecentralizedStableCoin(predictedEngine);
+
+ DSCEngine engine = new DSCEngine{salt: keccak256("predictEngine")}(tokenAddresses, priceFeedAddresses, address(dsc));
+
+ //dsc.transferOwnership(address(engine));
vm.stopBroadcast();
return (dsc, engine, config);
}

Moreover, corresponding changes should be made in the tests and deployment script. Note that in the new constructor, the owner's address needs to be provided.

This proposed changes improve the contract's efficiency, clarity and maintainability, and reduces the risk of potential issues or vulnerabilities.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.