zBanc - DynamicLiquidTokenConverter introduces breaking changes to the underlying bancorprotocol base
Introducing major changes to the complex underlying smart contract system that zBanc was forked from(bancorprotocol) may result in unnecessary complexity to be added. Complexity usually increases the attack surface and potentially introduces software misbehavior. Therefore, it is recommended to focus on reducing the changes to the base system as much as possible and comply with the interfaces and processes of the system instead of introducing diverging behavior.\\nFor example, `DynamicLiquidTokenConverterFactory` does not implement the `ITypedConverterFactory` while other converters do. Furthermore, this interface and the behavior may be expected to only perform certain tasks e.g. when called during an upgrade process. Not adhering to the base systems expectations may result in parts of the system failing to function for the new convertertype. Changes introduced to accommodate the custom behavior/interfaces may result in parts of the system failing to operate with existing converters. This risk is best to be avoided.\\nIn the case of `DynamicLiquidTokenConverterFactory` the interface is imported but not implemented at all (unused import). The reason for this is likely because the function `createConverter` in `DynamicLiquidTokenConverterFactory` does not adhere to the bancor-provided interface anymore as it is doing way more than “just” creating and returning a new converter. This can create problems when trying to upgrade the converter as the upgraded expected the shared interface to be exposed unless the update mechanisms are modified as well.\\nIn general, the factories `createConverter` method appears to perform more tasks than comparable type factories. It is questionable if this is needed but may be required by the design of the system. We would, however, highly recommend to not diverge from how other converters are instantiated unless it is required to provide additional security guarantees (i.e. the token was instantiated by the factory and is therefore trusted).\\nThe `ConverterUpgrader` changed in a way that it now can only work with the `DynamicLiquidTokenconverter` instead of the more generalized `IConverter` interface. This probably breaks the update for all other converter types in the system.\\nThe severity is estimated to be medium based on the fact that the development team seems to be aware of the breaking changes but the direction of the design of the system was not yet decided.\\nunused import\\nconverterType should be external as it is not called from within the same or inherited contracts\\n```\\nfunction converterType() public pure returns (uint16) {\\n    return 3;\\n}\\n```\\n\\ncreateToken can be external and is actually creating a token and converter that is using that token (the converter is not returned)(consider renaming to createTokenAndConverter)\\n```\\n{\\n    DSToken token = new DSToken(\\_name, \\_symbol, \\_decimals);\\n\\n    token.issue(msg.sender, \\_initialSupply);\\n\\n    emit NewToken(token);\\n\\n    createConverter(\\n      token,\\n      \\_reserveToken,\\n      \\_reserveWeight,\\n      \\_reserveBalance,\\n      \\_registry,\\n      \\_maxConversionFee,\\n      \\_minimumWeight,\\n      \\_stepWeight,\\n      \\_marketCapThreshold\\n    );\\n\\n    return token;\\n}\\n```\\n\\nthe upgrade interface changed and now requires the converter to be a `DynamicLiquidTokenConverter`. Other converters may potentially fail to upgrade unless they implement the called interfaces.\\n```\\n    function upgradeOld(DynamicLiquidTokenConverter \\_converter, bytes32 \\_version) public {\\n        \\_version;\\n        DynamicLiquidTokenConverter converter = DynamicLiquidTokenConverter(\\_converter);\\n        address prevOwner = converter.owner();\\n        acceptConverterOwnership(converter);\\n        DynamicLiquidTokenConverter newConverter = createConverter(converter);\\n       \\n        copyReserves(converter, newConverter);\\n        copyConversionFee(converter, newConverter);\\n        transferReserveBalances(converter, newConverter);\\n        IConverterAnchor anchor = converter.token();\\n       \\n        // get the activation status before it's being invalidated\\n        bool activate = isV28OrHigherConverter(converter) && converter.isActive();\\n       \\n        if (anchor.owner() == address(converter)) {\\n            converter.transferTokenOwnership(address(newConverter));\\n            newConverter.acceptAnchorOwnership();\\n        }\\n\\n        handleTypeSpecificData(converter, newConverter, activate);\\n        converter.transferOwnership(prevOwner);\\n       \\n        newConverter.transferOwnership(prevOwner);\\n       \\n        emit ConverterUpgrade(address(converter), address(newConverter));\\n    }\\n```\\n\\n```\\nfunction upgradeOld(\\n    IConverter \\_converter,\\n    bytes32 /\\* \\_version \\*/\\n) public {\\n    // the upgrader doesn't require the version for older converters\\n    upgrade(\\_converter, 0);\\n}\\n```\\n
It is a fundamental design decision to either follow the bancorsystems converter API or diverge into a more customized system with a different design, functionality, or even security assumptions. From the current documentation, it is unclear which way the development team wants to go.\\nHowever, we highly recommend re-evaluating whether the newly introduced type and components should comply with the bancor API (recommended; avoid unnecessary changes to the underlying system,) instead of changing the API for the new components. Decide if the new factory should adhere to the usually commonly shared `ITypedConverterFactory` (recommended) and if not, remove the import and provide a new custom shared interface. It is highly recommended to comply and use the bancor systems extensibility mechanisms as intended, keeping the previously audited bancor code in-tact and voiding unnecessary re-assessments of the security impact of changes.
```\\nfunction converterType() public pure returns (uint16) {\\n    return 3;\\n}\\n```\\n