Signed integer overflow in CCoinsViewCache::GetValueIn(…) #18858
Description
CCoinsViewCache::GetValueIn(…) performs money summation like this:
CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
{
if (tx.IsCoinBase())
return 0;
CAmount nResult = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
return nResult;
}Note that no check is done to make sure that the resulting nResult is such that it stays within the money bounds (MoneyRange(nResult)), or that the summation does not trigger a signed integer overflow.
Proof of concept output:
coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
GetValueIn = -9221444073709551616
Proof of concept code:
CMutableTransaction mutable_transaction;
mutable_transaction.vin.resize(4393);
Coin coin;
coin.out.nValue = MAX_MONEY;
assert(MoneyRange(coin.out.nValue));
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.coin = coin;
coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
CCoinsView backend_coins_view;
CCoinsViewCache coins_view_cache{&backend_coins_view};
CCoinsMap coins_map;
coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
coins_view_cache.BatchWrite(coins_map, {});
const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
std::cout << "GetValueIn = " << total_value_in << std::endl;As far as I can tell CCoinsViewCache::GetValueIn is unused outside of tests:
$ git grep GetValueIn ":(exclude)src/test/" ":(exclude)src/bench/"
src/coins.cpp:CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
src/coins.h: CAmount GetValueIn(const CTransaction& tx) const;
src/primitives/transaction.h: // GetValueIn() is a method on CCoinsViewCache, becauseI suggest we drop the unused CCoinsViewCache::GetValueIn(…).
Any objections? :)