PACTDelegate Manual Review Findings

PAC-01M: Absence of Veto Power

Type	Severity	Location
Standard Conformity		PACTDelegate.sol:L288-L292

Description:

The PACTDelegate contract contains no method to cancel a potentially harmful proposal.

Impact:

The absence of veto power could allow the DAO to take harmful action against itself due to oversight or any other similar human-error.

Example:

Copy
277/**
278 * @notice Cancels a proposal only if sender is the proposer, or proposer delegates dropped below proposal threshold
279 * @param _proposalId The id of the proposal to cancel
280 */
281function cancel(uint256 _proposalId) external {
282    require(
283        state(_proposalId) != ProposalState.Executed,
284        "PACT::cancel: cannot cancel executed proposal"
285    );
286
287    Proposal storage _proposal = proposals[_proposalId];
288    require(
289        msg.sender == _proposal.proposer ||
290            getPriorVotes(_proposal.proposer, sub256(block.number, 1)) < proposalThreshold,
291        "PACT::cancel: proposer above threshold"
292    );
293
294    _proposal.canceled = true;
295    for (uint256 i = 0; i < proposalTargets[_proposalId].length; i++) {
296        timelock.cancelTransaction(
297            proposalTargets[_proposalId][i],
298            proposalValues[_proposalId][i],
299            proposalSignatures[_proposalId][i],
300            proposalCalldatas[_proposalId][i],
301            _proposal.eta
302        );
303    }
304
305    emit ProposalCanceled(_proposalId);
306}

Recommendation:

We advise some form of control to be retained by the Impact Market team whereby they are able to veto a proposal. If decentralization is desired, an on-chain vote could be carried out to kick the Impact Market team from the system which they wouldn't be able to veto thus ensuring that the protocol is simultaneously safe-guarded and can become fully decentralized at any moment.

Alleviation:

The Impact Market states that 50% of the circulating supply is held by them and as such they have complete control over the DAO. This trait significantly centralizes the protocol and we advise against accumulation of majority voting power under a single entity. In any case, veto power is indeed achievable based on these circumstances and as such this particular exhibit can be considered as nullified.

PAC-02M: Complete Control of Contract Funds

Type	Severity	Location
Centralization Concern		PACTDelegate.sol:L567-L575

Description:

The transfer function permits the owner to transfer any asset held by the contract to an arbitrary party.

Example:

Copy
560/**
561 * @notice Transfers an amount of an ERC20 from this contract to an address
562 *
563 * @param _token address of the ERC20 token
564 * @param _to address of the receiver
565 * @param _amount amount of the transaction
566 */
567function transfer(
568    IERC20 _token,
569    address _to,
570    uint256 _amount
571) external onlyOwner nonReentrant {
572    _token.safeTransfer(_to, _amount);
573
574    emit TransferERC20(address(_token), _to, _amount);
575}

Recommendation:

We advise this trait of the system to be re-evaluated. Alternatively, we advise ownership of the contract to be given to the timelock implementation thus ensuring that the relevant owner-only functions are invoked responsibly.

Alleviation:

The owner of the contract has been transferred to timelock contract thereby making a centralized owner unable to transfer any funds to an arbitrary party, thus fully alleviating this exhibit.

PAC-03M: Significant Degree of Centralization

Type	Severity	Location
Centralization Concern		PACTDelegate.sol:L486, L501, L517, L533, L549, L571

Description:

The linked functions adjust sensitive contract variables affecting the overall governance mechanisms of the protocol yet are guarded by an onlyOwner modifier without any visible guarantees that the owner is a decentralized contract.

Example:

Copy
497/**
498 * @notice Owner function for setting the quorum votes
499 * @param _newQuorumVotes new quorum votes
500 */
501function _setQuorumVotes(uint256 _newQuorumVotes) external onlyOwner {
502    require(
503        _newQuorumVotes >= proposalThreshold,
504        "PACT::_setQuorumVotes: invalid quorum votes"
505    );
506
507    uint256 _oldQuorumVotes = votingDelay;
508    quorumVotes = _newQuorumVotes;
509
510    emit QuorumVotesSet(_oldQuorumVotes, _newQuorumVotes);
511}

Recommendation:

We advise the constructor of the contract to transfer ownership to the timelock implementation thereby guaranteeing that changes to the governance mechanism are brought forth by the protocol itself, increasing the operational security of the protocol.

Alleviation:

Ownership of the contract is now properly transferred to the _timelock address during its initialize function execution thereby guaranteeing that the onlyOwner functions are indeed meant to be invoked by the DAO itself and alleviating this exhibit. We would like to note that this degree of configurability for the system increases the captured value of a governance takeover and as such can cause complications during the early days of the protocol in case i.e. a whale purchases a significant amount of voting power when the token is not valuable.

PAC-04M: Potential Compromisation of Governance

Type	Severity	Location
Centralization Concern		PACTDelegate.sol:L549-L558

Description:

The releaseToken utilized for calculating voting power can be arbitrarily set multiple times.

Example:

Copy
545/**
546 * @notice Owner function for setting the release token
547 * @param _newReleaseToken new release token address
548 */
549function _setReleaseToken(IHasVotes _newReleaseToken) external onlyOwner {
550    require(
551        _newReleaseToken != token,
552        "PACT::_setReleaseToken: releaseToken and token must be different"
553    );
554    IHasVotes _oldReleaseToken = releaseToken;
555    releaseToken = _newReleaseToken;
556
557    emit ReleaseTokenSet(address(_oldReleaseToken), address(_newReleaseToken));
558}

Recommendation:

We advise the function to be invoke-able only once to prevent governance take-over attacks or malicious owner intent.

Alleviation:

The Impact Market team stated that this is a desired use case for the contract and that the variable is meant to be upgrade-able in the future. Given that this is a desired business use case, we consider this exhibit nullified although we still advise utmost care to be applied when handling the access control of this function.