It is possible to vote, finalise and execute the same proposal several times

[code423n4]

Handle

paulius.eth

Vulnerability details

Impact

function completeProposal which is the last step sets mapPID_finalised to true and resets mapPID_finalising to false. Function voteProposal only checks that mapPID_finalising is false, it does not check that the mapPID_finalised, thus the same proposal can be voted again, then finalized and executed.

Recommended Mitigation Steps

voteProposal should require that mapPID_finalised is false.

[strictly-scarce]

Valid attack path, although questionable if high-risk since funds-not-at-risk

[Mervyn853]

Our decision matrix for severity:

0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.

Recommended: 2

[0xBrian]

Unused mapPID_finalised addressed https://github.com/vetherasset/vaderprotocol-contracts/commit/6f961e6cd159e905ef53a5a067f956d21f8a46ee

[0xBrian]

Well, the unused mapPID_finalised was addressed, but this issue probably still remains.

[dmvt]

duplicate of #229