jl777 / chips3

MIT License
3 stars 31 forks source link

[security] possible flaw #50

Open ca333 opened 4 years ago

ca333 commented 4 years ago

https://github.com/jl777/chips3/blob/2ce1c3834a3f58ecff77a4552c3a789011d653eb/src/komodo_rpcblockchain.h#L33-L38

L38: we shouldn't assume it dpowconf'd unless NOTARIZED_HEIGHT > 0 happened

@mixa84 can you plz explain what i am missing cuz to me it does seem for "edge case bypassing" check should be rather

        static bool notarized = 0;
        if ( NOTARIZED_HEIGHT > 0 )
        {
            notarized = 1;
            if ( txheight < NOTARIZED_HEIGHT )
                return(numconfs);
            else return(1);
        } else if (notarized != 0)
            return(1);     
phm87 commented 4 years ago

Hello,

I copy pasted Mixa84's fix from komodo to chips. https://github.com/jl777/chips3/commit/a434ddb00cad574f16a4f53e2a54b8b924f5e4c5

The same change was performed to komodo. But then, jl777 added another change in the same code's place. https://github.com/KomodoPlatform/komodo/commit/ad5f6f8489a0d31b00b3865816f14e3b04c90027

Comparing jl777's code and code of ca333, I'm asking myself if hadnotarization (in jl777 code) should be initialized to 0 (as ca333 did here above)?

If the chain is not notarized, why do we return 1 instead of returning numconfs?

About the code } else if (notarized != 0) return(1); I don't see how notarized can be != 0 and reach this part of the code. notarized is initialized at 0 in ca333 code and it is set to 1 in the if with return in all cases. So I think that the else if will be always false.

ca333 commented 4 years ago

If the chain is not notarized, why do we return 1 instead of returning numconfs?

this is the reason why i tagged this - we return 1 conf when we "know" it was (1) before - however, there were edge cases where after hitting notarization the NOTARIZED_HEIGHT would be NOT >0 again (during syncing for example) in such a case its good to know if it had "status notarized" before - thus adding a static bool value to carry this info on during daemon runtime will solve such edge-cases.

So I think that the else if will be always false.

no - the "new" else if is basically for such an edge case where notarized is set to 1 and would keep this value during entire runtime - now in case that if ( NOTARIZED_HEIGHT > 0 ) does not trigger although was already > 0 during runtime we will keep the info and return (1) in the else if.

EDIT: updated above after clarification from James - returning 1 would be still <2 thus not notarized but confirmed. 0 is mempool and 1 "normal" confd.