Handle

TomFrenchBlockchain

Vulnerability details

Impact

Council can veto proposals to remove them to remain in power.

Proof of Concept

The Vader governance contract has the concept of a “council” which can unilaterally accept or reject a proposal. To prevent a malicious council preventing itself from being replaced by the token holders, the veto function checks the calldata for any proposal action directed at GovernorAlpha to see if it matches the changeCouncil function selector.

Note this is done by reading from the proposal.calldatas array.

<https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L568-L603&gt;

If we look at the structure of a proposal however we can see that the function selector is held (in the form of the signature) in the signatures array rather than being included in the calldata. The calldata array then holds just the function arguments for the call rather than specifying which function to call.

<https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L71-L72&gt;

<https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L356-L362&gt;

Indeed if we look at the TimeLock contract we see that the signature is hashed to calculate the function selector and is prepended onto the calldata.

<https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/Timelock.sol#L292-L299&gt;

Looking at the function signature of the changeCouncil we can see that the value that the veto function will check against this.changeCouncil.signature will be the first 4 bytes of an abi encoded address and so will always be zero no matter what function is being called.

<https://github.com/code-423n4/2021-12-vader/blob/fd2787013608438beae361ce1bb6d9ffba466c45/contracts/governance/GovernorAlpha.sol#L623&gt;

High risk as this issue gives the council absolute control over the DAO such that they cannot be removed.

Recommended Mitigation Steps

Hash the function signatures to calculate function selectors and then check those rather than the calldata.

This is something that should be picked up by a test suite however, I’d recommend writing tests to ensure that protections you add to the code have any affect and more broadly check that the code behaves as expected.

---

The text was updated successfully, but these errors were encountered:

All reactions