search

Sifchain / sifnode

SifNode - The future of Defi
Other
109 stars 118 forks source link

PRP-01 | Incorrect Prophecy Cleanup #2768

Closed juniuszhou closed 2 years ago

juniuszhou commented 2 years ago

From the audit, the judgment used to remove outdated prophecy is wrong.

juniuszhou commented 2 years ago

close after #2769 merged.

Brando753 commented 2 years ago

More Details

The function CleanUpProphecy() in x/oracle/keeper/prophecy.go should remove only outdated prophecies from the KVStore. However, the current implementation will remove all prophecies, including the latest one which is not expired, when the function CleanUpProphecy() is called.

        if prophecyInfo.BlockNumber-currentHeight > ProphecyLiftTime {
            storePrefix := append(types.SignaturePrefix, prophecyInfo.ProphecyId[:]...)
            store.Delete(storePrefix)
            storePrefix = k.getKeyViaNetworkDescriptorGlobalNonce(prophecyInfo.NetworkDescriptor, prophecyInfo.GlobalSequence)
            store.Delete(storePrefix)
        }

The condition prophecyInfo.BlockNumber - currentHeight > ProphecyLiftTime indicates that if the block number when the prophecy is created is greater, instead of less, than the current height by a certain number, the prophecy will be removed.

Because both prophecyInfo.BlockNumber and currentHeight are uint64, and prophecyInfo.BlockNumber should always be smaller than currentHeight, integer underflow would happen when calculating prophecyInfo.BlockNumber - currentHeight. Therefore, the condition that prophecyInfo.BlockNumber - currentHeight > ProphecyLiftTime will always be true, and all prophecies in the KVStore will be removed.