Zer0 - zBanc

Description

DynamicLiquidTokenConverter.reduceWeight

1. Calling reserveBalance to fetch the reserve balance for a given reserveToken might be redundant, as the value has already been fetched, and resides in the reserve local variable.
2. Function visibility can be changed to external instead of public.

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L130-L150

    function reduceWeight(IERC20Token _reserveToken)
        public
        validReserve(_reserveToken)
        ownerOnly
    {
        _protected();
        uint256 currentMarketCap = getMarketCap(_reserveToken);
        require(currentMarketCap > (lastWeightAdjustmentMarketCap.add(marketCapThreshold)), "ERR_MARKET_CAP_BELOW_THRESHOLD");

        Reserve storage reserve = reserves[_reserveToken];
        uint256 newWeight = uint256(reserve.weight).sub(stepWeight);
        uint32 oldWeight = reserve.weight;
        require(newWeight >= minimumWeight, "ERR_INVALID_RESERVE_WEIGHT");

        uint256 percentage = uint256(PPM_RESOLUTION).sub(newWeight.mul(PPM_RESOLUTION).div(reserve.weight));

        uint32 weight = uint32(newWeight);
        reserve.weight = weight;
        reserveRatio = weight;

        uint256 balance = reserveBalance(_reserveToken).mul(percentage).div(PPM_RESOLUTION);

• ConverterUpgrader.upgradeOld - Redundant casting of _converter.

zBanc/solidity/contracts/converter/ConverterUpgrader.sol:L96-L99

function upgradeOld(DynamicLiquidTokenConverter _converter, bytes32 _version) public {
    _version;
    DynamicLiquidTokenConverter converter = DynamicLiquidTokenConverter(_converter);
    address prevOwner = converter.owner();

Description

Consider using the best type available in the function arguments and declarations instead of accepting address and later casting it to the correct type.

Examples

This is only one of many examples.

zAuction/contracts/zAuction.sol:L22-L26

function init(address accountantaddress) external {
    require(!initialized);
    initialized = true;
    accountant = zAuctionAccountant(accountantaddress);
}

zAuction/contracts/zAuction.sol:L52-L54

IERC721 nftcontract = IERC721(nftaddress);
weth.transferFrom(bidder, msg.sender, bid);
nftcontract.transferFrom(msg.sender, bidder, tokenid);

zAuction/contracts/zAuction.sol:L40-L42

IERC721 nftcontract = IERC721(nftaddress);
accountant.Exchange(bidder, msg.sender, bid);
nftcontract.transferFrom(msg.sender, bidder, tokenid);

zAuction/contracts/zAuctionAccountant.sol:L60-L63

function SetZauction(address zauctionaddress) external onlyAdmin{
    zauction = zauctionaddress;
    emit ZauctionSet(zauctionaddress);
}

Description

reduceWeight calls _protected() in an attempt to protect from reentrant calls but this check is insufficient as it will only check for the locked statevar but never set it. A potential for direct reentrancy might be present when an erc-777 token is used as reserve.

It is assumed that the developer actually wanted to use the protected modifier that sets the lock before continuing with the method.

Examples

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L123-L128

function reduceWeight(IERC20Token _reserveToken)
    public
    validReserve(_reserveToken)
    ownerOnly
{
    _protected();

contract ReentrancyGuard {
    // true while protected code is being executed, false otherwise
    bool private locked = false;

    /**
      * @dev ensures instantiation only by sub-contracts
    */
    constructor() internal {}

    // protects a function against reentrancy attacks
    modifier protected() {
        _protected();
        locked = true;
        _;
        locked = false;
    }

    // error message binary size optimization
    function _protected() internal view {
        require(!locked, "ERR_REENTRANCY");
    }
}

Recommendation

To mitigate potential attack vectors from reentrant calls remove the call to _protected() and decorate the function with protected instead. This will properly set the lock before executing the function body rejecting reentrant calls.

Description

Check that the value in PPM is within expected bounds before updating system settings that may lead to functionality not working correctly. For example, setting out-of-bounds values for stepWeight or setMinimumWeight may make calls to reduceWeight fail. These values are usually set in the beginning of the lifecycle of the contract and misconfiguration may stay unnoticed until trying to reduce the weights. The settings can be fixed, however, by setting the contract inactive and updating it with valid settings. Setting the contract to inactive may temporarily interrupt the normal operation of the contract which may be unfavorable.

Examples

Both functions allow the full uint32 range to be used, which, interpreted as PPM would range from 0% to 4.294,967295%

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L75-L84

function setMinimumWeight(uint32 _minimumWeight)
    public
    ownerOnly
    inactive
{
    //require(_minimumWeight > 0, "Min weight 0");
    //_validReserveWeight(_minimumWeight);
    minimumWeight = _minimumWeight;
    emit MinimumWeightUpdated(_minimumWeight);
}

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L92-L101

function setStepWeight(uint32 _stepWeight)
    public
    ownerOnly
    inactive
{
    //require(_stepWeight > 0, "Step weight 0");
    //_validReserveWeight(_stepWeight);
    stepWeight = _stepWeight;
    emit StepWeightUpdated(_stepWeight);
}

Recommendation

Reintroduce the checks for _validReserveWeight to check that a percent value denoted in PPM is within valid bounds _weight > 0 && _weight <= PPM_RESOLUTION. There is no need to separately check for the value to be >0 as this is already ensured by _validReserveWeight.

Note that there is still room for misconfiguration (step size too high, min-step too high), however, this would at least allow to catch obviously wrong and often erroneously passed parameters early.

Description

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.

For 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.

In 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.

In 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).

The 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.

The 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.

Examples

• unused import

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverterFactory.sol:L6-L6

import "../../interfaces/ITypedConverterFactory.sol";

• converterType should be external as it is not called from within the same or inherited contracts

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverterFactory.sol:L144-L146

function converterType() public pure returns (uint16) {
    return 3;
}

• createToken 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)

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverterFactory.sol:L54-L74

{
    DSToken token = new DSToken(_name, _symbol, _decimals);

    token.issue(msg.sender, _initialSupply);

    emit NewToken(token);

    createConverter(
      token,
      _reserveToken,
      _reserveWeight,
      _reserveBalance,
      _registry,
      _maxConversionFee,
      _minimumWeight,
      _stepWeight,
      _marketCapThreshold
    );

    return token;
}

• the 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.

zBanc/solidity/contracts/converter/ConverterUpgrader.sol:L96-L122

    function upgradeOld(DynamicLiquidTokenConverter _converter, bytes32 _version) public {
        _version;
        DynamicLiquidTokenConverter converter = DynamicLiquidTokenConverter(_converter);
        address prevOwner = converter.owner();
        acceptConverterOwnership(converter);
        DynamicLiquidTokenConverter newConverter = createConverter(converter);
       
        copyReserves(converter, newConverter);
        copyConversionFee(converter, newConverter);
        transferReserveBalances(converter, newConverter);
        IConverterAnchor anchor = converter.token();
       
        // get the activation status before it's being invalidated
        bool activate = isV28OrHigherConverter(converter) && converter.isActive();
       
        if (anchor.owner() == address(converter)) {
            converter.transferTokenOwnership(address(newConverter));
            newConverter.acceptAnchorOwnership();
        }

        handleTypeSpecificData(converter, newConverter, activate);
        converter.transferOwnership(prevOwner);
       
        newConverter.transferOwnership(prevOwner);
       
        emit ConverterUpgrade(address(converter), address(newConverter));
    }

solidity/contracts/converter/ConverterUpgrader.sol:L95-L101

function upgradeOld(
    IConverter _converter,
    bytes32 /* _version */
) public {
    // the upgrader doesn't require the version for older converters
    upgrade(_converter, 0);
}

Recommendation

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.

However, 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.

Description

By default, a converter is active once the anchor ownership was transferred. This is true for converters that do not require to be properly set up with additional parameters before they can be used.

zBanc/solidity/contracts/converter/ConverterBase.sol:L272-L279

/**
  * @dev returns true if the converter is active, false otherwise
  *
  * @return true if the converter is active, false otherwise
*/
function isActive() public view virtual override returns (bool) {
    return anchor.owner() == address(this);
}

For a simple converter, this might be sufficient. If a converter requires additional setup steps (e.g. setting certain internal variables, an oracle, limits, etc.) it should return inactive until the setup completes. This is to avoid that users are interacting with (or even pot. frontrunning) a partially configured converter as this may have unexpected outcomes.

For example, the LiquidityPoolV2Converter overrides the isActive method to require additional variables be set (oracle) to actually be in active state.

zBanc/solidity/contracts/converter/types/liquidity-pool-v2/LiquidityPoolV2Converter.sol:L79-L85

  * @dev returns true if the converter is active, false otherwise
  *
  * @return true if the converter is active, false otherwise
*/
function isActive() public view override returns (bool) {
    return super.isActive() && address(priceOracle) != address(0);
}

Additionally, settings can only be updated while the contract is inactive which will be the case during an upgrade. This ensures that the owner cannot adjust settings at will for an active contract.

zBanc/solidity/contracts/converter/types/liquidity-pool-v2/LiquidityPoolV2Converter.sol:L97-L109

function activate(
    IERC20Token _primaryReserveToken,
    IChainlinkPriceOracle _primaryReserveOracle,
    IChainlinkPriceOracle _secondaryReserveOracle)
    public
    inactive
    ownerOnly
    validReserve(_primaryReserveToken)
    notThis(address(_primaryReserveOracle))
    notThis(address(_secondaryReserveOracle))
    validAddress(address(_primaryReserveOracle))
    validAddress(address(_secondaryReserveOracle))
{

The DynamicLiquidTokenConverter is following a different approach. It inherits the default isActive which sets the contract active right after anchor ownership is transferred. This kind of breaks the upgrade process for DynamicLiquidTokenConverter as settings cannot be updated while the contract is active (as anchor ownership might be transferred before updating values). To unbreak this behavior a new authentication modifier was added, that allows updates for the upgrade contradict while the contract is active. Now this is a behavior that should be avoided as settings should be predictable while a contract is active. Instead it would make more sense initially set all the custom settings of the converter to zero (uninitialized) and require them to be set and only the return the contract as active. The behavior basically mirrors the upgrade process of LiquidityPoolV2Converter.

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L44-L50

    modifier ifActiveOnlyUpgrader(){
      if(isActive()){
        require(owner == addressOf(CONVERTER_UPGRADER), "ERR_ACTIVE_NOTUPGRADER");
      }
      _;
    }

Pre initialized variables should be avoided. The marketcap threshold can only be set by the calling entity as it may be very different depending on the type of reserve (eth, token).

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L17-L20

uint32 public minimumWeight = 30000;
uint32 public stepWeight = 10000;
uint256 public marketCapThreshold = 10000 ether;
uint256 public lastWeightAdjustmentMarketCap = 0;

Here’s one of the setter functions that can be called while the contract is active (only by the upgrader contract but changing the ACL commonly followed with other converters).

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L67-L74

function setMarketCapThreshold(uint256 _marketCapThreshold)
    public
    ownerOnly
    ifActiveOnlyUpgrader
{
    marketCapThreshold = _marketCapThreshold;
    emit MarketCapThresholdUpdated(_marketCapThreshold);
}

Recommendation

Align the upgrade process as much as possible to how LiquidityPoolV2Converter performs it. Comply with the bancor API.

• override isActive and require the contracts main variables to be set.
• do not pre initialize the contracts settings to “some” values. Require them to be set by the caller (and perform input validation)
• mirror the upgrade process of LiquidityPoolV2Converter and instead of activate call the setter functions that set the variables. After setting the last var and anchor ownership been transferred, the contract should return active.

Description

The owner of the converter is allowed to reduce the converters weights once the marketcap surpasses a configured threshhold. The thresshold is configured on first deployment. The marketcap at the beginning of the call is calculated as reserveBalance / reserve.weight and stored as lastWeightAdjustmentMarketCap after reducing the weight.

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L130-L138

    function reduceWeight(IERC20Token _reserveToken)
        public
        validReserve(_reserveToken)
        ownerOnly
    {
        _protected();
        uint256 currentMarketCap = getMarketCap(_reserveToken);
        require(currentMarketCap > (lastWeightAdjustmentMarketCap.add(marketCapThreshold)), "ERR_MARKET_CAP_BELOW_THRESHOLD");

The reserveBalance can be manipulated by buying (adding reserve token) or selling liquidity tokens (removing reserve token). The success of a call to reduceWeight is highly dependant on the marketcap. A malicious actor may, therefore, attempt to grief calls made by the owner by sandwiching them with buy and sell calls in an attempt to (a) raise the barrier for the next valid payout marketcap or (b) temporarily lower the marketcap if they are a major token holder in an attempt to fail the reduceWeights call.

In both cases the griefer may incur some losses due to conversion errors, bancor fees if they are set, and gas spent. It is, therefore, unlikely that a third party may spend funds on these kinds of activities. However, the owner as a potential major liquid token holder may use this to their own benefit by artificially lowering the marketcap to the absolute minimum (old+threshold) by selling liquidity and buying it back right after reducing weights.

Description

According to the client the system was forked off bancor v0.6.18 (Oct 2020). The current version 0.6.x is v0.6.36 (Apr 2021).

Recommendation

It is recommended to check if relevant security fixes were released after v0.6.18 and it should be considered to rebase with the current stable release.

Description

DynamicContractRegistry is a wrapper registry that allows the zBanc to use the custom upgrader contract while still providing access to the normal bancor registry.

For this to work, the registry owner can add or override any registry setting. Settings that don’t exist in this contract are attempted to be retrieved from an underlying registry (contractRegistry).

zBanc/solidity/contracts/utility/DynamicContractRegistry.sol:L66-L70

function registerAddress(bytes32 _contractName, address _contractAddress)
    public
    ownerOnly
    validAddress(_contractAddress)
{

If the item does not exist in the registry, the request is forwarded to the underlying registry.

zBanc/solidity/contracts/utility/DynamicContractRegistry.sol:L52-L58

function addressOf(bytes32 _contractName) public view override returns (address) {
    if(items[_contractName].contractAddress != address(0)){
      return items[_contractName].contractAddress;
    }else{
      return contractRegistry.addressOf(_contractName);
    }
}

According to the documentation this registry is owned by zer0 admins and this means users have to trust zer0 admins to play fair.

To handle this, we deploy our own ConverterUpgrader and ContractRegistry owned by zer0 admins who can register new addresses

The owner of the registry (zer0 admins) can change the underlying registry contract at will. The owner can also add new or override any settings that already exist in the underlying registry. This may for example allow a malicious owner to change the upgrader contract in an attempt to potentially steal funds from a token converter or upgrade to a new malicious contract. The owner can also front-run registry calls changing registry settings and thus influencing the outcome. Such an event will not go unnoticed as events are emitted.

It should also be noted that itemCount will return only the number of items in the wrapper registry but not the number of items in the underlying registry. This may have an unpredictable effect on components consuming this information.

zBanc/solidity/contracts/utility/DynamicContractRegistry.sol:L36-L43

/**
  * @dev returns the number of items in the registry
  *
  * @return number of items
*/
function itemCount() public view returns (uint256) {
    return contractNames.length;
}

Recommendation

Require the owner/zer0 admins to be a DAO or multisig and enforce 2-step (notify->wait->upgrade) registry updates (e.g. by requiring voting or timelocks in the admin contract). Provide transparency about who is the owner of the registry as this may not be clear for everyone. Evaluate the impact of itemCount only returning the number of settings in the wrapper not taking into account entries in the subcontract (including pot. overlaps).

Description

getMarketCap calculates the reserve’s market capitalization as reserveBalance * 1e6 / weight where 1e6 should be expressed as the constant PPM_RESOLUTION.

Examples

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L157-L164

function getMarketCap(IERC20Token _reserveToken)
    public
    view
    returns(uint256)
{
    Reserve storage reserve = reserves[_reserveToken];
    return reserveBalance(_reserveToken).mul(1e6).div(reserve.weight);
}

Recommendation

Avoid hardcoding integer literals directly into source code when there is a better expression available. In this case 1e6 is used because weights are denoted in percent to base PPM_RESOLUTION (=100%).

Description

The system is forked frombancorprotocol/contracts-solidity. As such, it is very likely that security vulnerabilities reported to bancorprotocol upstream need to be merged into the zer0/zBanc fork if they also affect this codebase. There is also a chance that security fixes will only be available with feature releases or that the zer0 development team wants to merge upstream features into the zBanc codebase.

zBanc introduced converterType=3 for the DynamicLiquidTokenConverter as converterType=1 and converterType=2 already exist in the bancorprotocol codebase. Now, since it is unclear if DynamicLiquidTokenConverter will be merged into bancorprotocol there is a chance that bancor introduces new types that overlap with the DynamicLiquidTokenConverter converter type (3). It is therefore suggested to map the DynamicLiquidTokenConverter to a converterType that is unlikely to create an overlap with the system it was forked from. E.g. use converter type id 1001 instead of 3 (Note: converterType is an uint16).

Note that the current master of the bancorprotocol already appears to defined converterType 3 and 4: https://github.com/bancorprotocol/contracts-solidity/blob/5f4c53ebda784751c3a90b06aa2c85e9fdb36295/solidity/test/helpers/Converter.js#L51-L54

Examples

• The new custom converter

zBanc/solidity/contracts/converter/types/liquid-token/DynamicLiquidTokenConverter.sol:L50-L52

function converterType() public pure override returns (uint16) {
    return 3;
}

• ConverterTypes from the bancor base system

zBanc/solidity/contracts/converter/types/liquidity-pool-v1/LiquidityPoolV1Converter.sol:L71-L73

function converterType() public pure override returns (uint16) {
    return 1;
}

zBanc/solidity/contracts/converter/types/liquidity-pool-v2/LiquidityPoolV2Converter.sol:L73-L76

*/
function converterType() public pure override returns (uint16) {
    return 2;
}

Recommendation

Choose a converterType id for this custom implementation that does not overlap with the codebase the system was forked from. e.g. uint16(-1) or 1001 instead of 3 which might already be used upstream.

Description

DynamicContractRegistryClient is an exact copy of ContractRegistryClient. Avoid unnecessary code duplication.

< contract DynamicContractRegistryClient is Owned, Utils {
---
> contract ContractRegistryClient is Owned, Utils {