Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential overflow in crowdsale participation with indivisible tokens #247

Closed
dexX7 opened this issue Sep 19, 2015 · 2 comments
Closed

Potential overflow in crowdsale participation with indivisible tokens #247

dexX7 opened this issue Sep 19, 2015 · 2 comments

Comments

@dexX7
Copy link
Member

dexX7 commented Sep 19, 2015

CMPTransaction::logicHelper_CrowdsaleParticipation:

if (!isPropertyDivisible(property)) { // <-- the tokens "invested"
    nValue = nValue * 1e8; // <-- nValue is uint64_t
}

The cut-off is 92233720368, which is the largest number where nValue * 100000000 <= INT64_MAX, and sending more than 92233720368 indivisible tokens either causes an assertion violation or undefined behavior.

To be more specific:

Amount Master Core 0.0.9 Omni Core 0.0.10
0 to 92233720368 OK OK
92233720369 to 184467440737 OK assertion violation
184467440738 or more undefined undefined

The assertion violation is caused by the conversion of uint64_t to int64_t, which results in negative numbers in that range (rejected via ConvertTo256).

The results for numbers higher than 184467440738 are undefined, meaning: it's up to the compiler implementation how it's dealt with. On my system it seems to be 0, but it can really be anything between 0 and MAX.

The positive:

  • no new tokens can be created out of the blue
  • we can easily mirror the exact behavior of 0.0.9

The negative:

  • a buyer may not get the tokens he or she paid for

It's notable, even without overflow, that 92233720368 is the maximum number of divisible tokens that are "counted" anyway..

Potential fix:

int64_t adjusted = 0;

if (nValue > (INT64_MAX/100000000)) { // > 92233720368
    adjusted = INT64_MAX; // clip to max. allowed amount
} else {
    adjusted = nValue * 100000000;
}

edit: calculateFractional (for missed tokens) must be adjusted for the fix.

This is potentially consensus breaking.

@zathras-crypto
Copy link

Nice, thanks for this dude :)

Why don't we do the inflation of amounts once we're in uint256? We can avoid the modification of nvalue and overflow for INT64 should no longer be a concern?

With a simple patch such as zathras-crypto@33f3114, perhaps we may address this issue, #248 and #249 together?

Note - this is just calculateFundraiser, I haven't looked at calculateFractional yet...

I attempted to replicate your regtest with the patch:

omni_gettransaction af270a05abccd89c4ed7e554cf09c94da3cccad8f35851748f052f0b24e6844b
{
"txid" : "af270a05abccd89c4ed7e554cf09c94da3cccad8f35851748f052f0b24e6844b",
"fee" : "0.00000256",
"sendingaddress" : "n144os5tJMmfVP8SvRJD5TSZph7HMd7P7J",
"referenceaddress" : "msDqmnX8jpTRJdBevjqN5UQqfXUV9WU5aK",
"ismine" : true,
"version" : 0,
"type_int" : 0,
"type" : "Crowdsale Purchase",
"propertyid" : 3,
"divisible" : false,
"amount" : "1",
"purchasedpropertyid" : 4,
"purchasedpropertyname" : "Z_TestProperty",
"purchasedpropertydivisible" : false,
"purchasedtokens" : "1",
"issuertokens" : "0",
"valid" : true,
"blockhash" : "026a81dfeb9223dbf5ee5168556b60f01aa9066ee4820fbe4ee0dcf75d48252f",
"blocktime" : 1442730900,
"block" : 165,
"confirmations" : 1
}
omni_getcrowdsale 4 true
{
"propertyid" : 4,
"name" : "Z_TestProperty",
"active" : true,
"issuer" : "msDqmnX8jpTRJdBevjqN5UQqfXUV9WU5aK",
"propertyiddesired" : 3,
"tokensperunit" : "1",
"earlybonus" : 0,
"percenttoissuer" : 0,
"starttime" : 1442730900,
"deadline" : 1542729212,
"tokensissued" : "1",
"addedissuertokens" : "0",
"amountraised" : "1",
"participanttransactions" : [
{
"txid" : "af270a05abccd89c4ed7e554cf09c94da3cccad8f35851748f052f0b24e6844b",
"amountsent" : "1",
"participanttokens" : "1",
"issuertokens" : "0"
}
]
}

@dexX7
Copy link
Member Author

dexX7 commented Sep 20, 2015

That's pretty nice actually, and it even removes the artificial limit. Can you think of any downsides?

Note - this is just calculateFundraiser, I haven't looked at calculateFractional yet...

I'm very sure calculateFractional should be overhauled, because it basically re-runs all crowdsale participations to calculate totals.. as double. :/

But there is good news: unless I overlook something, we may almost completely remove it, because we already sumed all values.

int64_t missedTokens = (crowdsale.getUserCreated() * [bonus]) - crowdsale.getIssuerCreated();

Then another point I was wondering about: the "missed tokens" are only calculated, if a crowdsale expires, but not in case the crowdsale is manually closed. But it should, I assume? edit: nevermind, the missing tokens are calculated for manually closed crowdsales, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants