Frankencoin - fatherOfBlocks's results

contracts/Position.sol

• L138 - When the owner uses the adjust() function and defines the newCollateral, it is confusing that it is not clear how much is going to be transferred in the transferFrom, this can lead to several reverts because not knowing how much is going to be transferred specifically ( without consulting another function) I do not perform the corresponding approve, so perhaps it would be less confusing if the input reports how much it wants to add to the balance and use an event to report the current state of the balance.

• L132 - The adjust() function is responsible for updating the collateral and updating the Frankencoin tokens, therefore it is performing two tasks at the same time, something that does not follow good practices when writing code. It is recommended that there be two functions and that each one only handles one process.

contracts/MintingHub.sol

• L37 - There is commented code that should be removed as it can cause confusion.

• L88/124 - Lack of event emission after critical functions as openPosition() and clonePosition().

contracts/StablecoinBridge.sol

• L81 - To perform a revert, a require(false) is used, this is an incorrect use of require, the correct thing to do would be to use this: revert("unsupported token");

• L75 - The onTokenTransfer() function returns a bool and this is not very useful in this case, since the bool does not help us to know what was executed, nor what was the result, therefore the most useful thing would be for the function to be void and have the mintInternal() and burnInternal() functions raise events.

contracts/ERC20PermitLight.sol

• L35 - abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead.

• L65 - There is a commented line of code, this reduces the understanding of the code, therefore it should be removed.

contracts/Frankencoin.sol

• L266 - The name of the modifier is minterOnly() this is somewhat confusing the correct name should be onlyMinter().

contracts/Position.sol

• L104 - The NotQualified() error is created, but it is never used, this generates unnecessary gas waste in the deploy that can be avoided.

• L159 - With the implementation done, if price == newPrice, else would be executed anyway, this is unnecessary, therefore a better implementation could be: if(newPrice != price){ if (newPrice > price) restrictMinting(3 days); else checkCollateral(collateralBalance(), newPrice);

        price = newPrice;
        emitUpdate(); 
    }
  

contracts/MintingHub.sol

• L5/7 - IReserve and Ownable are imported, but they are never used, therefore, when performing the deploy, an unnecessary gas expense is generated and the code is less understandable when seeing all the complete code. It would be advisable to eliminate the imports.

• L181/188 - Public Functions Not Called By The Contract Should Be Declared External Instead In this specific case minBid() has two functions, one public and the other internal, this is a waste of gas, deploy and an unnecessary amount of complexity, the simplest thing would be to do just one public function or one external and another internal.

contracts/PositionFactory.sol

• L5 - IFrankencoin is imported, but it is never used, therefore it should be eliminated so as not to generate an extra expense in the deploy and less understanding when seeing all the complete code.

contracts/StablecoinBridge.sol

• L5 - IERC677Receiver is imported, but it is never used, therefore it should be removed so as not to generate extra expense in the deploy and less understanding when seeing all the complete code.

contracts/Frankencoin.sol

• L25 - A constant is created in storage, but it is only used in the suggestMinter() function, therefore it could be created only locally in that function, this would generate less gas cost.

• L144/286 - The balance - minReserve operation could be unchecked since the pre-check is done in L141, therefore we could save gas by avoiding this check. The same happens with _amount - reserveLeft, with the precheck in L282.

contracts/Equity.sol

• L7 - IERC677Receiver is imported, but it is never used, therefore it should be removed so as not to generate extra expense in the deploy and less understanding when seeing all the complete code.

• L39/46/59 - A constant is created in storage, but they are only used in the price(), checkQualified() and canRedeem() functions, therefore they could be created only locally in those functions, this would generate less cost of gas.

• L294 - The operation totalShares - shares could be unchecked since in L293 the pre-check is done, therefore we could save gas by avoiding this check.