Frankencoin - codeslide's results

Low Risk Issues List

[L-01] Unsafe casting of uints

Downcasting from uint256 in Solidity does not revert on overflow. This can result in undesired exploitation or bugs, since developers usually assume that overflows raise errors. OpenZeppelin's SafeCast restores this intuition by reverting the transaction when such an operation overflows. Using this library instead of the unchecked operations eliminates an entire class of bugs, so it’s recommended to always use it.

For example:

// Before
return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

// After
return toUint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

1. In Position.sol

   File: contracts/Position.sol
   
   187:    return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

2. In Equity.sol

   File: contracts/Equity.sol
   
   146:    totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);
   
   161:    voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance);

Non Critical Issues

[NC-01] Inconsistent code formatting

This code base exhibits inconsistencies that may be interpreted by a reader as a lack of attention to detail which could reduce confidence and trust in the protocol. The lack of consistent formatting also makes reading and maintaining the code more difficult.

To increase readability and maintainability, and to increase confidence in the protocol and the team behind it, the Solidity source code should use consistent formatting throughout.

Mitigation:

1. Follow the Solidity Style Guide (SSG)
2. Use forge fmt, prettier-solidity, or an equivalent solution to automatically format Solidity code.

There are many examples of inconsistent bracing styles, inconsistent horizontal whitespace, and generally not following the Solidity style guide. Below are some examples.

1. In Position.sol

   File: contracts/Position.sol
   
   // No spaces surrounding division operator on L98, but spaces are present on L122.
   // SSG: "Surround operators with a single space on either side."
   98:    uint256 reduction = (limit - minted - _minimum)/2;
   122:   return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
   
   // No space before opening curly brace on L121, but space is present on L160.
   // SSG: "The opening brace should be preceded by a single space."
   121:   if (afterFees){
   160:   if (newPrice > price) {

2. In Equity.sol

   File: contracts/Equity.sol
   
   // No spaces surrounding operators in for loop on L192, but are partially in place on L312.
   // SSG: "Surround operators with a single space on either side."
   192:    for (uint i=0; i<helpers.length; i++){
   312:    for (uint256 i = 0; i<addressesToWipe.length; i++){
   
   // No space before opening curly brace on L172, but space is present on L179.
   // SSG: "The opening brace should be preceded by a single space."
   172:   function anchorTime() internal view returns (uint64){
   179:   function votes(address holder) public view returns (uint256) {

3. In Frankencoin.sol

   File: contracts/Frankencoin.sol
   
   // Inconsistent indentation: The other contracts are using a four-space indent. This contract file uses a mix of two and three space indentation.
   
   // Three space indentation.
   64:    function name() override external pure returns (string memory){
   65:       return "Frankencoin";
   66:    }
   
   // Two space indentation.
   141:    if (balance <= minReserve){
   142:      return 0;
   143:    } else {
   144:      return balance - minReserve;
   145:    }

4. In ERC20PermitLight.sol

   File: contracts/ERC20PermitLight.sol
   
   // Space after start of single line comment.
   40:    // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
   
   // No space after start of single line comment.
   65:    //keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");

[NC-02] Use the complete name of data types

Subtle bugs can occur when just uint or int is used. For example, when hashing the signature of a function and use uint instead of uint256 for a parameter type.

Remediation recommendations:

1. Replace uint with uint256. Replace int with int256.
2. Use forge fmt, which will automatically change uint to uint256 and int to int256.

File: contracts/Equity.sol

91:    event Trade(address who, int amount, uint totPrice, uint newprice);

192:   for (uint i=0; i<helpers.length; i++){

196:   for (uint j=i+1; j<helpers.length; j++){

Gas Optimizations

[G-01] State variables should be cached

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read.

Saves 100 gas per instance

File: contracts/Frankencoin.sol

// 2nd call to totalSupply() which reads _totalSupply
85:    if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();

File: contracts/Position.sol

// mintingFeePPM and start
187:    return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

// minted
241:    if (amount > minted) revert RepaidTooMuch(amount - minted);

// minted
349:    uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF;

[G-02] Unnecessary computation

1. The call to zchf.equity() on line 292 is not necessary if the require() statement on 293 evaluates to false. Recommended to swap lines 292 and 293.

   File: contracts/Equity.sol
   
   291:    uint256 totalShares = totalSupply();
   292:    uint256 capital = zchf.equity();
   293:    require(shares + ONE_DEC18 < totalShares, "too many shares");

2. The require() at line 109 should be moved to the first line of the openPosition() function to avoid any unnecessary code execution.

   File: contracts/MintingHub.sol
   
   109:    require(_initialCollateral >= _minCollateral, "must start with min col");