CryptoCoderz / Espers

An Experimental Blockchain Project
https://espers.io/
MIT License
79 stars 52 forks source link

Fix integer overflow in POS reward calculation #43

Closed bitcoinx2 closed 3 years ago

bitcoinx2 commented 4 years ago

Relevant lines

./src/util/util.h:
    static const int64_t CENT = 1000000;
    static const int64_t COIN = 100000000;

./src/consensus/mining.h:
    static const int64_t COIN_YEAR_REWARD2 = 19 * CENT;

src/core/main.cpp: CTransaction::GetCoinAge()
    bnCentSecond += CBigNum(nValueIn) * (nTime-txPrev.nTime) / CENT;
    CBigNum bnCoinDay = bnCentSecond * CENT / COIN / (24 * 60 * 60);
    nCoinAge = bnCoinDay.getuint64();

src/core/blockparams.cpp:  GetProofOfStakeReward()
    int64_t nSubsidy = nCoinAge * COIN_YEAR_REWARD2 * 33 / (365 * 33 + 8);

src/core/wallet.cpp: CWallet::CreateCoinStake()
    int64_t nReward = GetProofOfStakeReward(nCoinAge, nFees);
    if (nReward <= 0)
        return false;

src/core/main.cpp: CBlock::ConnectBlock()
    int64_t nCalculatedStakeReward = GetProofOfStakeReward(nCoinAge, nFees);
    if (nStakeReward > nCalculatedStakeReward)
        return DoS(100, error("ConnectBlock() : coinstake pays too much(actual=%d vs calculated=%d)", nStakeReward, nCalculatedStakeReward));

Problem: Integer overflow in nSubsidy calculation

In the current phase, COIN_YEAR_REWARD2 is being used.

int64_t nSubsidy = nCoinAge * 19 * 1000000 * 33 / (365 * 33 + 8);

nCoinAge * 19 * 1000000 * 33 must be smaller than Int64 max (9223372036854775807) to avoid integer overflow! If the intermediate value gets larger than this, it will turn negative and the remaining calculation (/ (365 * 33 + 8)) will operate on the negative value.

nCoinAge * 19 * 1000000 * 33 < 9223372036854775807
nCoinAge < 9223372036854775807 / (19 * 1000000 * 33)
nCoinAge < 14710322228

If we do the math (see math section), we see that nCoinAge = coins * ageInDays

coins * ageInDays < 14710322228

So, for example if a 40.3 million UTXO is older than 1 year, it will not be able to stake because an integer overflow will happen!

For an 80 million UTXO, the threshold is just a half year etc..

Solution: Use CBigNum for intermediate values

blockparams.cpp:
    CBigNum bnSubsidy = CBigNum(nCoinAge) * COIN_YEAR_REWARD2 * 33 / (365 * 33 + 8);
    ...
    int64_t nSubsidy = bnSubsidy.getuint64();

This does not solve the general problem that nSubsidy as a whole might overflow, so an assert statement was added that checks for this case. Practically, this will not happen because it would only overflow at a coinAge of 1.77+14, or holding 1 billion coins for 485 years. ;)

Consensus effects

This patch will cause a hard fork because old clients will not accept staking transactions with high coinAge.

Appendix: Math for nCoinAge = coins * ageInDays

nCoinAge = bnCoinDay
= bnCentSecond * CENT / COIN / (24 * 60 * 60)
= bnCentSecond * 1000000 / 100000000 / (24 * 60 * 60)
= bnCentSecond / (100 * 24 * 60 * 60)
= CBigNum(nValueIn) * (nTime-txPrev.nTime) / CENT / (100 * 24 * 60 * 60)
= CBigNum(nValueIn) * (nTime-txPrev.nTime) / (1000000 * 100 * 24 * 60 * 60)

nTime-txPrev.nTime is age in seconds

= nValueIn * ageInSeconds / (1000000 * 100 * 24 * 60 * 60)

nValueIn is satoshi, so nValueIn = coins * 1e8

= coins * 100000000 * ageInSeconds / (1000000 * 100 * 24 * 60 * 60)
= coins * ageInSeconds / (24 * 60 * 60)

One day is 24*60*60 seconds, so ageInSeconds = ageInDays * (24*60*60)

= coins * ageInDays * (24*60*60) / (24 * 60 * 60)
= coins * ageInDays
CryptoCoderz commented 4 years ago

I'll set this up on a toggle for testing then merge this