CREDITSCOM / node

Credits Node is the main module that provide an opportunity to run a node and participate in CREDITS blockchain network.
https://developers.credits.com/
GNU Affero General Public License v3.0
151 stars 16 forks source link

Minor bugs in default token contract #11

Open tkoen93 opened 5 years ago

tkoen93 commented 5 years ago

Describe the bug The default token contract that is being used with the web wallet contains some minor bugs. Should be pretty easy to fix, but the default token contract needs some revision.

Thanks to Steffen in tech chat for pointing this issue out as well.

What currently needs to be done is the correct use of 'totalCoins' (TotalSupply). Upon deploying a token contract, the entire supply will be added to the public key of the contract owner. That means that the contract address does not contain any of that just created token.

When someone tries to burn some tokens, the burn function is called and defined as follows in the default token contract:

public boolean burn(String amount) {
        contractIsNotFrozen();
        BigDecimal decimalAmount = toBigDecimal(amount);
        if (!initiator.equals(owner))
            throw new RuntimeException("can not burn tokens! The wallet " + initiator + " is not owner");
        if (totalCoins.compareTo(decimalAmount) < 0) totalCoins = ZERO.setScale(decimal, ROUND_DOWN);
        else totalCoins = totalCoins.subtract(decimalAmount);
        return true;
    }

This function allows only the owner of the contract to use this function. Next to that, when 'burning' tokens, only the 'totalCoins' is being reduced. 'totalCoins' is equal to the total supply on the monitor. There are no tokens actually being burned, as nothing gets subtracted from an account.

With the current burn function it's actually possible to enter an amount below zero. This way you can even increase the totalCoins / total supply.

I suggest the following as a solution for this function.

public boolean burn(String amount) {
        contractIsNotFrozen();
        BigDecimal decimalAmount = toBigDecimal(amount);
        BigDecimal sourceBalance = getTokensBalance(initiator);
        if(decimalAmount.compareTo(ZERO) < 0) {
            throw new IllegalArgumentException("the amount cannot be negative");
        }
        if (sourceBalance.compareTo(decimalAmount) < 0) {
            throw new RuntimeException("the wallet " + initiator + " doesn't have enough tokens to burn");
        }
            totalCoins = totalCoins.subtract(decimalAmount);
            balances.put(initiator, sourceBalance.subtract(decimalAmount));
        return true;
    }

In my opinion anyone should be able to burn a token they possess. It's the users own responsibility when they burn tokens.

The code above checks if the entered amount is below zero. If so, it throws an error. After that, the entered amount is being checked against the actual balance of the token. When someone tries to burn more tokens than they possess, it throw another error. That causes these transactions to fail.

When it passes both checks, the 'totalCoins' is reduced. Next to that, the burned tokens get subtracted from the account that executed this transaction, causing them to actually being 'burned'.

totalCoins is currently also being used in the public void payable() function. Might need to be checked as well.

To Reproduce

rustem-s commented 5 years ago

Default token contract changed.

tkoen93 commented 5 years ago

@rustem-s Hi, thanks for reply.

Tried to deploy the contract via the web wallet, some empty alert. Copied the contract into the wallet-desktop.jar and getting the following error: image Looks like there is no checkNegative function in the contract, so it doesn't work at the moment.

ghost commented 5 years ago

please clear your browser cache and try again

tkoen93 commented 5 years ago

Able to deploy it now because there is a checkNegative function.

Still other issues. totalCoins is set to 0 as default. freeCoins has been added and set to 10 million as default? tokenCost doesn't do anything in the payable function.

I understand that this is not really an issue, as the deployer is responsible for the contract they deploy. But as an offered default contract I wouldn't recommend using this default contract at it's current state.