GovernorAlpha Manual Review Findings

GAA-01M: Fully Manipulateable Governance

Description:

The current governance implementation is fully prone to flash-loan manipulation as it relies on spot-evaluations of a staked balance which can be unstaked at no penalty. While the MasterPlatypus implementation is properly synchronized whenever a burn or mint operation occurs on the VePtp contract, the governance implementation is not. In the current state, an attacker can detect when a proposal is nearing completion and flash-loan a large amount of Ptp, stake them in the VePtp contract, cast their vote and then unstake them returning the flash-loan.

Example:

Copy
351function _castVote(
352    address voter,
353    uint256 proposalId,
354    bool support
355) internal {
356    require(state(proposalId) == ProposalState.Active, 'GovernorAlpha::_castVote: voting is closed');
357    Proposal storage proposal = proposals[proposalId];
358    if (proposal.startBlock == 0) {
359        proposal.startBlock = block.number - 1;
360        emit StartBlockSet(proposalId, block.number);
361    }
362    Receipt storage receipt = proposal.receipts[voter];
363    require(receipt.hasVoted == false, 'GovernorAlpha::_castVote: voter already voted');
364    uint256 votes = vePtp.balanceOf(voter);
365
366    if (support) {
367        proposal.forVotes = add256(proposal.forVotes, votes);
368    } else {
369        proposal.againstVotes = add256(proposal.againstVotes, votes);
370    }
371
372    receipt.hasVoted = true;
373    receipt.support = support;
374    receipt.votes = votes;
375
376    emit VoteCast(voter, proposalId, support, votes);
377}

Recommendation:

We advise the voting system to be revamped and potentially utilize historical balances rather than spot balances on the VeERC20Upgradeable implementation. An example of this can be seen within the Compound implementation of the Comp token.

Alleviation:

A new voting power mechanism was introduced to the codebase by the Platypus team that mandates the user has staked their tokens for a significant amount of time, thereby prohibiting opportunistic governance manipulation attack vectors from being profitable. As a result, we consider this exhibit adequately dealt with.

GAA-02M: Insecure Elliptic Curve Recovery Mechanism

Description:

The ecrecover function is a low-level cryptographic function that should be utilized after appropriate sanitizations have been enforced on its arguments, namely on the s and v values. This is due to the inherent trait of the curve to be symmetrical on the x-axis and thus permitting signatures to be replayed with the same x value (r) but a different y value (s).

Example:

Copy
334function castVoteBySig(
335    uint256 proposalId,
336    bool support,
337    uint8 v,
338    bytes32 r,
339    bytes32 s
340) public {
341    bytes32 domainSeparator = keccak256(
342        abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this))
343    );
344    bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
345    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
346    address signatory = ecrecover(digest, v, r, s);
347    require(signatory != address(0), 'GovernorAlpha::castVoteBySig: invalid signature');
348    return _castVote(signatory, proposalId, support);
349}

Recommendation:

We advise them to be sanitized by ensuring that v is equal to either 27 or 28 (v ∈ {27, 28}) and to ensure that s is existent in the lower half order of the elliptic curve (0 < s < secp256k1n ÷ 2 + 1) by ensuring it is less than 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1. A reference implementation of those checks can be observed in the ECDSA library of OpenZeppelin and the rationale behind those restrictions exists within Appendix F of the Yellow Paper.

Alleviation:

The Platypus team considered this exhibit but opted not to apply a remediation for it.

GAA-03M: Impossible Cancellation of Proposals

Description:

The codebase is based on the Compound governance implementation and in the introduced delta the ability for the guardian to cancel proposals has been removed.

Example:

Copy
266function cancel(uint256 proposalId) public {
267    ProposalState state_ = state(proposalId);
268    require(state_ != ProposalState.Executed, 'GovernorAlpha::cancel: cannot cancel executed proposal');
269
270    Proposal storage proposal = proposals[proposalId];
271    require(
272        vePtp.balanceOf(proposal.proposer) < proposalThreshold(),
273        'GovernorAlpha::cancel: proposer above threshold'
274    );
275
276    proposal.canceled = true;
277    for (uint256 i = 0; i < proposal.targets.length; i++) {
278        timelock.cancelTransaction(
279            proposal.targets[i],
280            proposal.values[i],
281            proposal.signatures[i],
282            proposal.calldatas[i],
283            proposal.eta
284        );
285    }
286
287    emit ProposalCanceled(proposalId);
288}

Recommendation:

We advise this trait of the system to be evaluated as it can have devastating consequences. The reason each vote has a delay is to allow for the guardian to react in time in case a malicious vote has been submitted. We advise this attribute to be re-instated for at least the early days of the DAO after which the guardian can abdicate themselves.

Alleviation:

The Platypus team considered this exhibit but opted not to apply a remediation for it.