Frankencoin - matrix_0wl's results

Non Critical Issues

[NC-1] ADD TO INDEXED PARAMETER FOR COUNTABLE EVENTS

Description:

Add to indexed parameter for countable Events

Proof Of Concept

File: contracts/ERC20.sol

158:         emit Transfer(sender, recipient, amount);

186:         emit Transfer(address(0), recipient, amount);

205:         emit Transfer(account, address(0), amount);

223:         emit Approval(owner, spender, value);

[NC-2] ADD A TIMELOCK TO CRITICAL FUNCTIONS

Description:

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Proof Of Concept

File: contracts/Ownable.sol

32:         setOwner(newOwner);

39:     function setOwner(address newOwner) internal {

File: contracts/Position.sol

54:         setOwner(_owner);

78:         setOwner(owner);

[NC-3] Be explicit declaring types

Description:

Instead of uint use uint256

Proof Of Concept

File: contracts/Equity.sol

91:     event Trade(address who, int amount, uint totPrice, uint newprice); // amount pos or neg for mint or redemption

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

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

[NC-4] GENERATE PERFECT CODE HEADERS EVERY TIME

Description:

I recommend using header for Solidity code layout and readability: https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

[NC-5] USE A SINGLE FILE FOR ALL SYSTEM-WIDE CONSTANTS

Description:

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values).

This will help with readability and easier maintenance for future changes.

Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution.

Proof Of Concept

File: contracts/Equity.sol

39:     uint32 public constant VALUATION_FACTOR = 3;

59:     uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

File: contracts/Frankencoin.sol

25:    uint256 public constant MIN_FEE = 1000 * (10**18);

File: contracts/MintingHub.sol

20:     uint256 public constant OPENING_FEE = 1000 * 10**18;

26:     uint32 public constant CHALLENGER_REWARD = 20000; // 2%

[NC-6] SOLIDITY COMPILER VERSIONS MISMATCH

Description:

The project is compiled with different versions of Solidity, which is not recommended because it can lead to undefined behaviors.

It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks.

Proof Of Concept

File: contracts/ERC20.sol

12: pragma solidity ^0.8.0;

File: contracts/ERC20PermitLight.sol

5: pragma solidity ^0.8.0;

File: contracts/Equity.sol

4: pragma solidity >=0.8.0 <0.9.0;

File: contracts/Frankencoin.sol

2: pragma solidity ^0.8.0;

File: contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

File: contracts/MintingHub.sol

2: pragma solidity ^0.8.0;

File: contracts/Ownable.sol

9: pragma solidity ^0.8.0;

File: contracts/Position.sol

2: pragma solidity ^0.8.0;

File: contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

File: contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

[NC-7] SIGNATURE MALLEABILITY OF EVM’S ECRECOVER()

Description:

The function calls the Solidity ecrecover() function directly to verify the given signatures. However, the ecrecover() EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

References: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57.

While this is not immediately exploitable, this may become a vulnerability if used elsewhere.

Proof Of Concept

File: contracts/ERC20PermitLight.sol

33:             address recoveredAddress = ecrecover(

Recommended Mitigation Steps:

Use the ecrecover function from OpenZeppelin’s ECDSA library for signature verification. (Ensure using a version > 4.7.3 for there was a critical bug >= 4.1.0 < 4.7.3).

[NC-8] FUNCTIONS, PARAMETERS AND VARIABLES IN SNAKE CASE

Description:

Use camel case for all functions, parameters and variables and snake case for constants.

Proof Of Concept

File: contracts/ERC20PermitLight.sol

61:     function DOMAIN_SEPARATOR() public view returns (bytes32) {

[NC-9] FOR MODERN AND MORE READABLE CODE; UPDATE IMPORT USAGES

Description:

Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it.

This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.

Proof Of Concept

File: contracts/ERC20.sol

14: import "./IERC20.sol";

15: import "./IERC677Receiver.sol";

File: contracts/ERC20PermitLight.sol

7: import "./ERC20.sol";

File: contracts/Equity.sol

6: import "./Frankencoin.sol";

7: import "./IERC677Receiver.sol";

8: import "./ERC20PermitLight.sol";

9: import "./MathUtil.sol";

10: import "./IReserve.sol";

File: contracts/Frankencoin.sol

4: import "./ERC20PermitLight.sol";

5: import "./Equity.sol";

6: import "./IReserve.sol";

7: import "./IFrankencoin.sol";

File: contracts/MintingHub.sol

4: import "./IERC20.sol";

5: import "./IReserve.sol";

6: import "./IFrankencoin.sol";

7: import "./Ownable.sol";

8: import "./IPosition.sol";

File: contracts/Position.sol

4: import "./IERC20.sol";

5: import "./IPosition.sol";

6: import "./IReserve.sol";

7: import "./IFrankencoin.sol";

8: import "./Ownable.sol";

9: import "./MathUtil.sol";

File: contracts/PositionFactory.sol

4: import "./Position.sol";

5: import "./IFrankencoin.sol";

File: contracts/StablecoinBridge.sol

4: import "./IERC20.sol";

5: import "./IERC677Receiver.sol";

6: import "./IFrankencoin.sol";

Recommended Mitigation Steps:

import {contract1 , contract2} from "filename.sol"; OR Use specific imports syntax per solidity docs recommendation.

[NC-10] LARGE MULTIPLES OF TEN SHOULD USE SCIENTIFIC NOTATION

Description:

Use (e.g. 1e6) rather than decimal literals (e.g. 100000), for better code readability.

Proof Of Concept

File: contracts/Equity.sol

211:         if (_votes * 10000 < QUORUM * totalVotes()) revert NotQualified();

File: contracts/Frankencoin.sol

118:       return minterReserveE6 / 1000000;

205:       uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;

239:       return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM); // 41 / (1-18%) = 50

239:       return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM); // 41 / (1-18%) = 50

File: contracts/MathUtil.sol

11:     uint256 internal constant THRESH_DEC18 =  10000000000000000;//0.01

File: contracts/MintingHub.sol

26:     uint32 public constant CHALLENGER_REWARD = 20000; // 2%

[NC-11] FOR EXTENDED “USING-FOR” USAGE, USE THE LATEST PRAGMA VERSION

Description:

https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/

Proof Of Concept

Recommended Mitigation Steps:

Use solidity pragma version min. 0.8.13

File: contracts/ERC20.sol

12: pragma solidity ^0.8.0;

File: contracts/ERC20PermitLight.sol

5: pragma solidity ^0.8.0;

File: contracts/Equity.sol

4: pragma solidity >=0.8.0 <0.9.0;

File: contracts/Frankencoin.sol

2: pragma solidity ^0.8.0;

File: contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

File: contracts/MintingHub.sol

2: pragma solidity ^0.8.0;

File: contracts/Ownable.sol

9: pragma solidity ^0.8.0;

File: contracts/Position.sol

2: pragma solidity ^0.8.0;

File: contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

File: contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

[NC-12] ALLOWS MALLEABLE SECP256K1 SIGNATURES

Description:

Here, the ecrecover() method doesn’t check the s range.

Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check.

Since an order can only be confirmed once and its hash is saved, there doesn’t seem to be a serious danger in existing use cases.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7201e6707f6631d9499a569f492870ebdd4133cf/contracts/utils/cryptography/ECDSA.sol#L138-L149

Proof Of Concept

File: contracts/ERC20PermitLight.sol

33:             address recoveredAddress = ecrecover(
                    keccak256(
                    abi.encodePacked(
                        "\x19\x01",
                        DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
                                bytes32(0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9),
                                owner,
                                spender,
                                value,
                                nonces[owner]++,
                                deadline
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );

[NC-13] MISSING EVENT FOR CRITICAL PARAMETER CHANGE

Description:

Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-access-control

Proof Of Concept

File: contracts/ERC20.sol

158:         emit Transfer(sender, recipient, amount);

186:         emit Transfer(address(0), recipient, amount);

205:         emit Transfer(account, address(0), amount);

223:         emit Approval(owner, spender, value);

[NC-14] MISSING FEE PARAMETER VALIDATION

Description:

Some fee parameters of functions are not checked for invalid values. Validate the parameters.

Proof Of Concept

File: contracts/Frankencoin.sol

165:    function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {

File: contracts/MintingHub.sol

59:             function openPosition(

88:             function openPosition(

File: contracts/Position.sol

50:             constructor(address _owner, address _hub, address _zchf, address _collateral,

File: contracts/PositionFactory.sol

13:           function createNewPosition(address _owner, address _zchf, address _collateral, uint256 _minCollateral,

[NC-15] NO SAME VALUE INPUT CONTROL

Proof Of Concept

File: contracts/ERC20.sol

60:         decimals = _decimals;

File: contracts/Frankencoin.sol

60:       MIN_APPLICATION_PERIOD = _minApplicationPeriod;

File: contracts/Position.sol

56:         hub = _hub;

60:         mintingFeePPM = _mintingFeePPM;

63:         challengePeriod = _challengePeriod;

82:         limit = _limit;

Recommended Mitigation Steps

Add code like this; if (oracle == _oracle revert ADDRESS_SAME();

[NC-16] FUNCTION WRITING THAT DOES NOT COMPLY WITH THE SOLIDITY STYLE GUIDE

Description:

Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private, within a grouping, place the view and pure functions last

Context:

contracts/Position.sol contracts/MintingHub.sol contracts/Equity.sol contracts/Frankencoin.sol contracts/ERC20.sol contracts/StablecoinBridge.sol

[NC-17] USE A MORE RECENT VERSION OF SOLIDITY

Description:

For security, it is best practice to use the latest Solidity version. For the security fix list in the versions; https://github.com/ethereum/solidity/blob/develop/Changelog.md

Proof Of Concept

File: contracts/ERC20.sol

12: pragma solidity ^0.8.0;

File: contracts/ERC20PermitLight.sol

5: pragma solidity ^0.8.0;

File: contracts/Equity.sol

4: pragma solidity >=0.8.0 <0.9.0;

File: contracts/Frankencoin.sol

2: pragma solidity ^0.8.0;

File: contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

File: contracts/MintingHub.sol

2: pragma solidity ^0.8.0;

File: contracts/Ownable.sol

9: pragma solidity ^0.8.0;

File: contracts/Position.sol

2: pragma solidity ^0.8.0;

File: contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

File: contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

[NC-18] Unused imports

Proof Of Concept

File: contracts/Equity.sol

7: import "./IERC677Receiver.sol";

File: contracts/Frankencoin.sol

6: import "./IReserve.sol";

File: contracts/MintingHub.sol

4: import "./IERC20.sol";

5: import "./IReserve.sol";

7: import "./Ownable.sol";

File: contracts/Position.sol

4: import "./IERC20.sol";

6: import "./IReserve.sol";

7: import "./IFrankencoin.sol";

File: contracts/PositionFactory.sol

4: import "./Position.sol";

File: contracts/StablecoinBridge.sol

4: import "./IERC20.sol";

5: import "./IERC677Receiver.sol";

[NC-19] FOR FUNCTIONS AND VARIABLES FOLLOW SOLIDITY STANDARD NAMING CONVENTIONS

Description:

Solidity’s standard naming convention for internal and private functions and variables (apart from constants): the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

Solidity’s standard naming convention for internal and private constants variables: the snake_case format starting with an underscore (_mixedCase starting with an underscore) and use ALL_CAPS for naming them.

Proof Of Concept

File: contracts/ERC20.so

97:     function allowanceInternal(address owner, address spender) internal view virtual returns (uint256) {

File: contracts/Equity.sol

41:     uint256 private constant MINIMUM_EQUITY = 1000 * ONE_DEC18;

46:     uint32 private constant QUORUM = 300;

51:     uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

75:     uint192 private totalVotesAtAnchor;  // Total number of votes at the anchor time, see comment on the um

76:     uint64 private totalVotesAnchorTime; // 40 Bit for the block number, 24 Bit sub-block time resolution

88:     mapping (address => uint64) private voteAnchor; // 40 Bit for the block number, 24 Bit sub-block time resolution

144:     function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {

157:     function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){

172:     function anchorTime() internal view returns (uint64){

225:     function canVoteFor(address delegate, address owner) internal view returns (bool) {

266:     function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {

File: contracts/Frankencoin.sol

39:    uint256 private minterReserveE6;

102:    function allowanceInternal(address owner, address spender) internal view override returns (uint256) {

File: contracts/MathUtil.sol

10:     uint256 internal constant ONE_DEC18 = 10**18;

11:     uint256 internal constant THRESH_DEC18 =  10000000000000000;//0.01

File: contracts/MintingHub.sol

28:     IPositionFactory private immutable POSITION_FACTORY; // position contract to clone

188:     function minBid(Challenge storage challenge) internal view returns (uint256) {

287:     function returnCollateral(Challenge storage challenge, bool postpone) internal {

File: contracts/Ownable.sol

39:     function setOwner(address newOwner) internal {

45:     function requireOwner(address sender) internal view {

File: contracts/Position.sol

169:     function collateralBalance() internal view returns (uint256){

193:     function mintInternal(address target, uint256 amount, uint256 collateral_) internal {

202:     function restrictMinting(uint256 period) internal {

232:     function repayInternal(uint256 burnable) internal {

240:     function notifyRepaidInternal(uint256 amount) internal {

268:     function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {

282:     function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {

286:     function emitUpdate() internal {

File: contracts/PositionFactory.sol

37:     function createClone(address target) internal returns (address result) {

File: contracts/StablecoinBridge.sol

49:     function mintInternal(address target, uint256 amount) internal {

67:     function burnInternal(address zchfHolder, address target, uint256 amount) internal {

Low Issues

[L-1] Arbitrary Jump with Function Type Variable

Description:

Function types are supported in Solidity. This means that a variable of type function can be assigned to a function with a matching signature. The function can then be called from the variable just like any other function. Users should not be able to change the function variable, but in some cases this is possible.

If the smart contract uses certain assembly instructions, mstore for example, an attacker may be able to point the function variable to any other function. This may give the attacker the ability to break the functionality of the contract, and perhaps even drain the contract funds.

Since inline assembly is a way to access the EVM at a low level, it bypasses many important safety features. So it's important to only use assembly if it is necessary and properly understood.

SWC Registry

Proof Of Concept

File: contracts/PositionFactory.sol

39:         assembly {
                let clone := mload(0x40)
                mstore(clone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
                mstore(add(clone, 0x14), targetBytes)
                mstore(add(clone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
                result := create(0, clone, 0x37)
        }

[L-2] DOS WITH BLOCK GAS LIMIT

Description:

When smart contracts are deployed or functions inside them are called, the execution of these actions always requires a certain amount of gas, based of how much computation is needed to complete them. The Ethereum network specifies a block gas limit and the sum of all transactions included in a block can not exceed the threshold.

Programming patterns that are harmless in centralized applications can lead to Denial of Service conditions in smart contracts when the cost of executing a function exceeds the block gas limit. Modifying an array of unknown size, that increases in size over time, can lead to such a Denial of Service condition.

Code4arena example

Proof Of Concept

File: contracts/Equity.sol

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

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

312:      for (uint256 i = 0; i<addressesToWipe.length; i++){

[L-3] AVOID TRANSFER()/SEND() AS REENTRANCY MITIGATIONS

Description:

Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes may break deployed contracts. Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://swcregistry.io/docs/SWC-134

Proof Of Concept

File: contracts/Equity.sol

279:         zchf.transfer(target, proceeds);

File: contracts/MintingHub.sol

204:             zchf.transfer(challenge.bidder, challenge.bid); // return old bid

211:             challenge.position.collateral().transfer(msg.sender, challenge.size);

263:             IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);

268:             zchf.transfer(owner, effectiveBid - fundsNeeded);

272:         zchf.transfer(challenge.challenger, reward); // pay out the challenger reward

284:         IERC20(collateral).transfer(target, amount);

294:             challenge.position.collateral().transfer(challenge.challenger, challenge.size); // return the challenger's collateral

File: contracts/Position.sol

253:             IERC20(token).transfer(target, amount);

269:         IERC20(collateral).transfer(target, amount);

File: contracts/StablecoinBridge.sol

69:         chf.transfer(target, amount);

Recommended Mitigation Steps:

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

[L-4] AVOID USING LOW CALL FUNCTION ECRECOVER

Description:

Use OZ library ECDSA that its battle tested to avoid classic errors.

https://docs.openzeppelin.com/contracts/4.x/api/utils#ECDSA

Proof Of Concept

File: contracts/ERC20PermitLight.sol

33:             address recoveredAddress = ecrecover(

[L-5] CRITICAL ADDRESS CHANGES SHOULD USE TWO-STEP PROCEDURE

Description:

The critical procedures should be two step process.

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. (see here and here)

Proof Of Concept

File: contracts/Ownable.sol

31:     function transferOwnership(address newOwner) public onlyOwner {

39:     function setOwner(address newOwner) internal {

Recommended Mitigation Steps:

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

[L-6] DECODING AN IPFS HASH USING A FIXED HASH FUNCTION AND LENGTH OF THE HASH

Description:

An IPFS hash specifies the hash function and length of the hash in the first two bytes of the hash.

Although SHA256 is 32 bytes and is currently the most common IPFS hash function, other content could use a hash function that is larger than 32 bytes. The current implementation limits the usage to the SHA256 hash function and a hash length of 32 bytes.

Proof Of Concept

Recommended Mitigation Steps:

Consider using a more generic implementation that can handle different hash functions and lengths and allow the user to choose.

File: contracts/ERC20PermitLight.sol

41:                                 bytes32(0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9),

66:                     bytes32(0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218),

File: contracts/PositionFactory.sol

41:             mstore(clone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)

43:             mstore(add(clone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)

43:             mstore(add(clone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)

[L-7] ECRECOVER MAY RETURN EMPTY ADDRESS

Description:

There is a common issue that ecrecover returns empty (0x0) address when the signature is invalid. function _verifySigner should check that before returning the result of ecrecover.

Proof Of Concept

File: contracts/ERC20PermitLight.sol

33:             address recoveredAddress = ecrecover(

Recommended Mitigation Steps:

See the solution here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/cryptography/ECDSA.sol#L68

[L-8] LOSS OF PRECISION DUE TO ROUNDING

Proof Of Concept

File: contracts/Equity.sol

109:         return VALUATION_FACTOR * zchf.equity() * ONE_DEC18 / totalSupply();

File: contracts/Frankencoin.sol

166:       uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000; // rounding down is fine

205:       uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;

209:          return theoreticalReserve * currentReserve / minterReserve();

239:       return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM); // 41 / (1-18%) = 50

File: contracts/MathUtil.sol

32:         return _a * _b / ONE_DEC18;

36:         return (_a * ONE_DEC18) / _b ;

File: contracts/MintingHub.sol

165:             (challenge.bid * splitOffAmount) / challenge.size

189:         return (challenge.bid * 1005) / 1000;

265:         uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;

File: contracts/Position.sol

80:         price = _mint * ONE_DEC18 / _coll;

122:             return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;

124:             return totalMint * (1000_000 - reserveContribution) / 1000_000;

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

[L-9] OWNER CAN RENOUNCE OWNERSHIP

Description:

Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The Openzeppelin’s Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

Proof Of Concept

File: contracts/MintingHub.sol

7: import "./Ownable.sol";

File: contracts/Ownable.sol

19: contract Ownable {

File: contracts/Position.sol

8: import "./Ownable.sol";

14: contract Position is Ownable, IPosition, MathUtil {

Recommended Mitigation Steps:

We recommend to either reimplement the function to disable it or to clearly specify if it is part of the contract design.

[L-10] REQUIRE MESSAGES ARE TOO SHORT AND UNCLEAR

Description:

The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory. These error descriptions are very important in the debug features of DApps like Tenderly. Error definitions should be added to the require block, not exceeding 32 bytes.

Proof Of Concept

Recommended Mitigation Steps:

Error definitions should be added to the require block, not exceeding 32 bytes or we should use custom errors

File: contracts/ERC20.sol

152:         require(recipient != address(0));

180:         require(recipient != address(0));

File: contracts/Equity.sol

194:             require(current != sender);

195:             require(canVoteFor(sender, current));

197:                 require(current != helpers[j]); // ensure helper unique

276:         require(canRedeem(msg.sender));

310:         require(zchf.equity() < MINIMUM_EQUITY);

File: contracts/MintingHub.sol

158:         require(challenge.challenger != address(0x0));

171:         require(challenge.size >= min);

172:         require(copy.size >= min);

254:         require(challenge.challenger != address(0x0));

File: contracts/Position.sol

53:         require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values

[L-11] Revert if ecrecover is address(0)

Description:

On ERC20PermitLight.sol#L33 add a revert that triggers if the response is address(0), this means that signature its not valid.

Proof Of Concept

File: contracts/ERC20PermitLight.sol

33:             address recoveredAddress = ecrecover(

[L-12] BLOCK VALUES AS A PROXY FOR TIME

Description:

Contracts often need access to time values to perform certain types of functionality. Values such as block.timestamp, and block.number can give you a sense of the current time or a time delta, however, they are not safe to use for most purposes.

In the case of block.timestamp, developers often attempt to use it to trigger time-dependent events. As Ethereum is decentralized, nodes can synchronize time only to some degree. Moreover, malicious miners can alter the timestamp of their blocks, especially if they can gain advantages by doing so. However, miners cant set a timestamp smaller than the previous one (otherwise the block will be rejected), nor can they set the timestamp too far ahead in the future. Taking all of the above into consideration, developers cant rely on the preciseness of the provided timestamp.

As for block.number, considering the block time on Ethereum is generally about 14 seconds, it`s possible to predict the time delta between blocks. However, block times are not constant and are subject to change for a variety of reasons, e.g. fork reorganisations and the difficulty bomb. Due to variable block times, block.number should also not be relied on for precise calculations of time.

Reference: https://swcregistry.io/docs/SWC-116

Reference: (https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/timestamp-dependence.md)

Proof Of Concept

File: contracts/ERC20PermitLight.sol

30:         require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");

File: contracts/Equity.sol

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

File: contracts/Frankencoin.sol

88:       minters[_minter] = block.timestamp + _applicationPeriod;

153:       if (block.timestamp > minters[_minter]) revert TooLate();

294:       return minters[_minter] != 0 && block.timestamp >= minters[_minter];

File: contracts/MintingHub.sol

144:         challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));

201:         if (block.timestamp >= challenge.end) revert TooLate();

217:             uint256 earliestEnd = block.timestamp + 30 minutes;

240:         return challenges[_challengeNumber].end > block.timestamp;

255:         require(block.timestamp >= challenge.end, "period has not ended");

File: contracts/Position.sol

64:         start = block.timestamp + initPeriod; // one week time to deny the position

110:         if (block.timestamp >= start) revert TooLate();

183:         uint256 time = block.timestamp;

203:         uint256 horizon = block.timestamp + period;

305:         if (block.timestamp >= expiration){

367:         if (block.timestamp > expiration) revert Expired();

374:         if (block.timestamp <= cooldown) revert Hot();

File: contracts/StablecoinBridge.sol

29:         horizon = block.timestamp + 52 weeks;

50:         require(block.timestamp <= horizon, "expired");

[L-13] UNSAFE CAST

Description:

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Proof Of Concept

File: contracts/Equity.sol

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

File: contracts/Position.sol

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

Recommended Mitigation Steps:

Use a safeCast from Open Zeppelin or increase the type length.

[L-14] USE _SAFEMINT INSTEAD OF _MINT

Description:

According to openzepplin’s ERC721, the use of _mint is discouraged, use safeMint whenever possible.

https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-mint-address-uint256-

Proof Of Concept

File: contracts/ERC20.sol

179:     function _mint(address recipient, uint256 amount) internal virtual {

File: contracts/Equity.sol

248:         _mint(from, shares);

File: contracts/Frankencoin.sol

167:       _mint(_target, usableMint);

168:       _mint(address(reserve), _amount - usableMint); // rest goes to equity as reserves or as fees

173:       _mint(_target, _amount);

286:          _mint(msg.sender, _amount - reserveLeft);

Recommended Mitigation Steps:

Use _safeMint whenever possible instead of _mint

Gas Optimizations

[GAS-1] BEFORE SOME FUNCTIONS, WE SHOULD CHECK SOME VARIABLES FOR POSSIBLE GAS SAVE

Description:

Before transfer, we should check for amount being 0 so the function doesnt run when its not gonna do anything.

Proof Of Concept

File: contracts/ERC20.sol

85:     function transfer(address recipient, uint256 amount) public virtual override returns (bool) {

125:     function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {

151:     function _transfer(address sender, address recipient, uint256 amount) internal virtual {

162:     function transferAndCall(address recipient, uint256 amount, bytes calldata data) external override returns (bool) {

File: contracts/Equity.sol

279:         zchf.transfer(target, proceeds);

File: contracts/MintingHub.sol

108:         zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);

110:         IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);

129:         existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);

142:         IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);

204:             zchf.transfer(challenge.bidder, challenge.bid); // return old bid

210:             zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);

211:             challenge.position.collateral().transfer(msg.sender, challenge.size);

225:             zchf.transferFrom(msg.sender, address(this), _bidAmountZCHF);

263:             IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);

268:             zchf.transfer(owner, effectiveBid - fundsNeeded);

272:         zchf.transfer(challenge.challenger, reward); // pay out the challenger reward

284:         IERC20(collateral).transfer(target, amount);

294:             challenge.position.collateral().transfer(challenge.challenger, challenge.size); // return the challenger's collateral

File: contracts/Position.sol

138:             collateral.transferFrom(msg.sender, address(this), newCollateral - colbal);

228:         IERC20(zchf).transferFrom(msg.sender, address(this), amount);

253:             IERC20(token).transfer(target, amount);

269:         IERC20(collateral).transfer(target, amount);

352:         internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update

File: contracts/StablecoinBridge.sol

45:         chf.transferFrom(msg.sender, address(this), amount);

69:         chf.transfer(target, amount);

[GAS-2] SETTING THE CONSTRUCTOR TO PAYABLE

Description:

Saves ~13 gas per instance

Proof Of Concept

File: contracts/ERC20.sol

59:     constructor(uint8 _decimals) {

File: contracts/Equity.sol

93:     constructor(Frankencoin zchf_) ERC20(18) {

File: contracts/Frankencoin.sol

59:    constructor(uint256 _minApplicationPeriod) ERC20(18){

File: contracts/MintingHub.sol

54:     constructor(address _zchf, address factory) {

File: contracts/Position.sol

50:     constructor(address _owner, address _hub, address _zchf, address _collateral,

File: contracts/StablecoinBridge.sol

26:     constructor(address other, address zchfAddress, uint256 limit_){

[GAS-3] DOS WITH BLOCK GAS LIMIT

Description:

When smart contracts are deployed or functions inside them are called, the execution of these actions always requires a certain amount of gas, based of how much computation is needed to complete them. The Ethereum network specifies a block gas limit and the sum of all transactions included in a block can not exceed the threshold.

Programming patterns that are harmless in centralized applications can lead to Denial of Service conditions in smart contracts when the cost of executing a function exceeds the block gas limit. Modifying an array of unknown size, that increases in size over time, can lead to such a Denial of Service condition.

Code4arena example

Proof Of Concept

File: contracts/Equity.sol

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

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

312:      for (uint256 i = 0; i<addressesToWipe.length; i++){

Recommended Mitigation Steps:

Caution is advised when you expect to have large arrays that grow over time. Actions that require looping across the entire data structure should be avoided.

If you absolutely must loop over an array of unknown size, then you should plan for it to potentially take multiple blocks, and therefore require multiple transactions.

[GAS-4] USE FUNCTION INSTEAD OF MODIFIERS

Proof Of Concept

File: contracts/Frankencoin.sol

266:    modifier minterOnly() {

File: contracts/MintingHub.sol

115:     modifier validPos(address position) {

File: contracts/Ownable.sol

49:     modifier onlyOwner() {

File: contracts/Position.sol

366:     modifier alive() {

373:     modifier noCooldown() {

380:     modifier noChallenge() {

387:     modifier onlyHub() {

[GAS-5] MAKING CONSTANT VARIABLES PRIVATE WILL SAVE GAS DURING DEPLOYMENT

Description:

When constants are marked public, extra getter functions are created, increasing the deployment cost. Marking these functions private will decrease gas cost. One can still read these variables through the source code. If they need to be accessed by an external contract, a separate single getter function can be used to return all constants as a tuple. There are four instances of public constants.

Proof Of Concept

File: contracts/Equity.sol

39:     uint32 public constant VALUATION_FACTOR = 3;

59:     uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

File: contracts/Frankencoin.sol

25:    uint256 public constant MIN_FEE = 1000 * (10**18);

File: contracts/MintingHub.sol

20:     uint256 public constant OPENING_FEE = 1000 * 10**18;

26:     uint32 public constant CHALLENGER_REWARD = 20000; // 2%

[GAS-6] THE INCREMENT IN FOR LOOP POSTCONDITION CAN BE MADE UNCHECKED

Description:

This is only relevant if you are using the default solidity checked arithmetic.

The for loop postcondition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks.One can manually do this.

Source

Proof Of Concept

File: contracts/Equity.sol

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

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

312:         for (uint256 i = 0; i<addressesToWipe.length; i++){

[GAS-7] PROPER DATA TYPES

Description:

In Solidity, some data types are more expensive than others. It’s important to be aware of the most efficient type that can be used. Here are a few rules about data types.

Type uint should be used in place of type string whenever possible.

Type uint256 takes less gas to store than uint8

Type bytes should be used over byte[]

If the length of bytes can be limited, use the lowest amount possible from bytes1 to bytes32.

Type bytes32 is cheaper to use than type string and bytes.

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity.

Source

Fixed size variables are always cheaper than dynamic ones.

Source

Most of the time it will be better to use a mapping instead of an array because of its cheaper operations.

Proof Of Concept

File: contracts/ERC20.sol

51:     uint8 public immutable override decimals;

59:     constructor(uint8 _decimals) {

File: contracts/ERC20PermitLight.sol

26:         uint8 v,

File: contracts/Equity.sol

39:     uint32 public constant VALUATION_FACTOR = 3;

46:     uint32 private constant QUORUM = 300;

51:     uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

75:     uint192 private totalVotesAtAnchor;  // Total number of votes at the anchor time, see comment on the um

76:     uint64 private totalVotesAnchorTime; // 40 Bit for the block number, 24 Bit sub-block time resolution

88:     mapping (address => uint64) private voteAnchor; // 40 Bit for the block number, 24 Bit sub-block time resolution

161:             voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); // new example anchor is only 21 / 11 = 1 block in the past

172:     function anchorTime() internal view returns (uint64){

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

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

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

File: contracts/Frankencoin.sol

165:    function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {

204:    function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {

223:    function burnFrom(address payer, uint256 targetTotalBurnAmount, uint32 _reservePPM) external override minterOnly returns (uint256) {

251:    function burnWithReserve(uint256 _amountExcludingReserve, uint32 _reservePPM) external override minterOnly returns (uint256) {

File: contracts/MintingHub.sol

26:     uint32 public constant CHALLENGER_REWARD = 20000; // 2%

91:         uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {

309:         uint32 _mintingFeePPM,

311:         uint32 _reserve

File: contracts/Position.sol

38:     uint32 public immutable mintingFeePPM;

39:     uint32 public immutable reserveContribution; // in ppm

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

File: contracts/PositionFactory.sol

15:         uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reserve)

38:         bytes20 targetBytes = bytes20(target);

[GAS-8] USING SOLIDITY VERSION 0.8.19 WILL PROVIDE AN OVERALL GAS OPTIMIZATION

Proof Of Concept

File: contracts/ERC20.sol

12: pragma solidity ^0.8.0;

File: contracts/ERC20PermitLight.sol

5: pragma solidity ^0.8.0;

File: contracts/Equity.sol

4: pragma solidity >=0.8.0 <0.9.0;

File: contracts/Frankencoin.sol

2: pragma solidity ^0.8.0;

File: contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

File: contracts/MintingHub.sol

2: pragma solidity ^0.8.0;

File: contracts/Ownable.sol

9: pragma solidity ^0.8.0;

File: contracts/Position.sol

2: pragma solidity ^0.8.0;

File: contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

File: contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

[GAS-9] TERNARY OPERATION IS CHEAPER THAN IF-ELSE STATEMENT

Description:

There are instances where a ternary operation can be used instead of if-else statement. In these cases, using ternary operation will save modest amounts of gas.

Proof Of Concept

File: contracts/Frankencoin.sol

141:  if (balance <= minReserve){
        return 0;
      } else {
        return balance - minReserve;
      }

207:   if (currentReserve < minterReserve()){
         // not enough reserves, owner has to take a loss
         return theoreticalReserve * currentReserve / minterReserve();
      } else {
         return theoreticalReserve;
      }

File: contracts/Position.sol

121:    if (afterFees){
            return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
        } else {
            return totalMint * (1000_000 - reserveContribution) / 1000_000;
        }

160:    if (newPrice > price) {
            restrictMinting(3 days);
        } else {
            checkCollateral(collateralBalance(), newPrice);
        }

184:    if (time >= exp){
            return 0;
        } else {
            return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
        }

250:    if (token == address(collateral)){
            withdrawCollateral(target, amount);
        } else {
            IERC20(token).transfer(target, amount);
        }

[GAS-10] USAGE OF UINT/INT SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

Description:

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html

Proof Of Concept

File: contracts/ERC20.sol

51:     uint8 public immutable override decimals;

59:     constructor(uint8 _decimals) {

File: contracts/ERC20PermitLight.sol

26:         uint8 v,

File: contracts/Equity.sol

39:     uint32 public constant VALUATION_FACTOR = 3;

46:     uint32 private constant QUORUM = 300;

51:     uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

76:     uint64 private totalVotesAnchorTime; // 40 Bit for the block number, 24 Bit sub-block time resolution

88:     mapping (address => uint64) private voteAnchor; // 40 Bit for the block number, 24 Bit sub-block time resolution

161:             voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); // new example anchor is only 21 / 11 = 1 block in the past

172:     function anchorTime() internal view returns (uint64){

173:         return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

File: contracts/Frankencoin.sol

165:    function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {

194:    function burn(uint256 amount, uint32 reservePPM) external override minterOnly {

204:    function calculateAssignedReserve(uint256 mintedAmount, uint32 _reservePPM) public view returns (uint256) {

223:    function burnFrom(address payer, uint256 targetTotalBurnAmount, uint32 _reservePPM) external override minterOnly returns (uint256) {

235:    function calculateFreedAmount(uint256 amountExcludingReserve /* 41 */, uint32 reservePPM /* 20% */) public view returns (uint256){

251:    function burnWithReserve(uint256 _amountExcludingReserve, uint32 _reservePPM) external override minterOnly returns (uint256) {

File: contracts/MintingHub.sol

26:     uint32 public constant CHALLENGER_REWARD = 20000; // 2%

62:         uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {

91:         uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {

260:         (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);

309:         uint32 _mintingFeePPM,

311:         uint32 _reserve

File: contracts/Position.sol

38:     uint32 public immutable mintingFeePPM;

39:     uint32 public immutable reserveContribution; // in ppm

52:         uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {

181:     function calculateCurrentFee() public view returns (uint32) {

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

329:     function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {

File: contracts/PositionFactory.sol

15:         uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reserve)

[GAS-11] UNNECESSARY LIBRARIES

Description:

Libraries are often only imported for a small number of uses, meaning that they can contain a significant amount of code that is redundant to your contract. If you can safely and effectively implement the functionality imported from a library within your contract, it is optimal to do so. Source

Proof Of Concept

File: contracts/Equity.sol

7: import "./IERC677Receiver.sol";

File: contracts/Frankencoin.sol

6: import "./IReserve.sol";

File: contracts/MintingHub.sol

4: import "./IERC20.sol";

5: import "./IReserve.sol";

7: import "./Ownable.sol";

File: contracts/Position.sol

4: import "./IERC20.sol";

6: import "./IReserve.sol";

7: import "./IFrankencoin.sol";

File: contracts/PositionFactory.sol

4: import "./Position.sol";

File: contracts/StablecoinBridge.sol

4: import "./IERC20.sol";

5: import "./IERC677Receiver.sol";

[GAS-12] USE BYTES32 INSTEAD OF STRING

Description:

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Proof Of Concept

File: contracts/Equity.sol

97:     function name() override external pure returns (string memory) {

101:     function symbol() override external pure returns (string memory) {

File: contracts/Frankencoin.sol

52:    event MinterApplied(address indexed minter, uint256 applicationPeriod, uint256 applicationFee, string message);

53:    event MinterDenied(address indexed minter, string message);

64:    function name() override external pure returns (string memory){

68:    function symbol() override external pure returns (string memory){

83:    function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {

152:    function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {

File: contracts/Position.sol

43:     event PositionDenied(address indexed sender, string message); // emitted if closed by governance

109:     function deny(address[] calldata helpers, string calldata message) public {