code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/governance/GovernorCharlie.sol#L347

Vulnerability details

Bug description

GovernorCharlie is the governance contract which owns the whole Amphora system. Users with enough voting power or which are whitelisted are able to make proposals, which consist in a set of internal calls, that GovernorCharlie is to execute. Other users are able to vote for, against or abstain from voting on proposals. At the end of the voting period, a proposal is either denied or queued for execution, and in that case is later executed. When making a proposal, for each call in the proposal, the proposer specifices:

target : the target of the call.
value : the amount of ETH to be sent with the call.
signature : an optional string containing the invoked method's signature.
calldata : the call's calldata, in case no signature is provided it's expected to already contain the method selector.

Given that GovernorCharlie never validates the provided data, I'd assume that task is left to all governance participants to take it upon themselves to verify that a proposal effectively carries out the intended actions and doesn't act maliciously towards the protocol.

Noting this lack of validation and the existence of invisible characters in unicode encoding, I explored the possibility for an attacker to craft a string containing such invisible characters which looks like the signature to a certain method (i.e. setProposalThreshold(uint256)) but in reality, when its selector is calculated, it results in the selector of a different method (i.e. setNewToken(address)).

Using a python script I was able to mine such spoofed signature which looks like the signature setProposalThreshold(uint256) but whose selector matches that of setNewToken(address). In the PoC I'll provide this spoofed signature and show how its selector clashes with 0x5ed411e5, setNewToken(address)'s selector.

Impact

Coming up with the worst case scenario and the largest impact it could have on the protocol isn't easy, especially given the short time provided to complete this contest. In the PoC I show how a malicious proposal looks to be calling GovernorCharlie.setProposalThreshold setting such threshold to X tokens, when in reality it'll end up calling GovernorCharlie.setNewToken setting the new governance token's address to address(X). In the case where X is an address that does not implement IERC20Votes, the proposal and voting processes will be bricked forever, only allowing to queue and execute proposals that were successful before the attack occurred, as calls to address(X) that expect return data will revert, like IERC20Votes.getPriorVotes. Other possible impacts that I wasn't able to analyze deeply would be:

Transferring/burning funds from GovernorCharlie, which is supposed to act as the protocol's treasury.
Transferring/burning ownership of any contract in the system, as GovernorCharlie owns them all.

Proof of Concept

Given that cast sig returns an error when given a string with invisible characters like U+200E, I've deployed a minimal contract on Goerli to verify the EVM behaves as expected when given these special characters, find it here. Notice it calculates the selector of a given string in the same way as GovernorCharlie.executeTransaction does.

Given setProposalThreshold(uint256) and setNewToken(address), we can easily verify their selectors to be, respectively, 0xece40cc1 and 0x5ed411e5.

Now, consider the string provided in the following gist. Copying the whole gist content and inserting it into this tool you are able to see the non-printable characters it contains. Copying the whole gist content and checking the EVM's behaviour when extracting that string's selector by using my deployed contract you will see the calculated selector is 0x5ed411e5, which matches setNewToken(address)'s selector.

Thus, if a governance proposal with target = GovernorCharlie and such signature field were to pass and be executed, all voters would expect the new proposalThreshold to be updated to a given value X, but in reality it would update GovernorCharli.amph to address(X), bricking governance proposals and voting forever.

Mitigating factors

I've found 2 main mitigating factors which can't be ignored:

While the honest and spoofed signatures may look the same on screen, a keen observer looking into the bytes of the calldata would easily recognize that a spoofed signature has been provided, as the byte stream length wouldn't match that of the printed string, raising questions and ultimately leading to detection and cancellation of the proposal. I argue that while this seems reasonable for small proposals, within a multicall proposal with up to 10 independent calls, this malicious signature is a lot harder to detect and could very well be overlooked, going unnoticed.
If a human actor were to visualize all signatures provided in every call in every proposal, scanning them with a tool to detect non-printable characters like the one I provided above, this type of attack would be swiftly detected and shutdown.

To both arguments, I argue that although this may seem a simple process now that this attack vector has been uncovered, no mention has been made that any countermeasures will be carried out, hence why I think it was important to report this very unique attack vector.

Taking into consideration the factors listed above and the fact that it takes only 1 slip to wreak havoc on the system with this attack vector, I classify it as a high, even though I'd say it has medium/low likelihood of being succesfully carried out.

Tools Used

Python Goerli Etherscan Manual review

Recommended Mitigation Steps

Given that it's clearly not practical to sanitize the provided signature fields onchain or to rely on a human actor check every provided field for every proposal, I advise to either switch the string signature field to a bytes4 selector field, in order to leave no room for the ambiguity underlined by this report or to remove this possibility completely and expect a proposal's call calldata field to always incorporate the selector.

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

0xShaito marked the issue as disagree with severity

0xShaito commented 1 year ago

This issue is present in the governor bravo implementation that we are using and it's the job of the governance to check proposals carefully before approving.

c4-judge commented 1 year ago

dmvt changed the severity to 2 (Med Risk)

dmvt commented 1 year ago

This is quite clever and well written. I don't think it's particularly likely, though, and as the sponsor pointed out, it's the job of governance to notice this type of thing and not approve a malicious proposal. The impact is very high, however, I've downgraded it to medium to reflect the external factor (governance members not doing their job) and corresponding likelihood.

c4-judge commented 1 year ago

dmvt marked the issue as selected for report

c4-sponsor commented 1 year ago

0xShaito marked the issue as sponsor disputed

0xShaito commented 1 year ago

After reviewing again we notice that Compound is still using the same implementation and the issue is fully inherited from them. Also, this can be fixed through UI by showing non-white characters anyways in the governance page

dmvt commented 1 year ago

I will think on the severity and will probably downgrade to QA. The fact that the issue is inherited doesn't make it invalid. However, as I read through the issue again, it seems like the emergency mitigation would be to simply deploy new contracts.

thebrittfactor commented 1 year ago

For transparency, the judge requested to downgrade to QA.