alterDelegateeOnBehalf() can potentially be used maliciously in a way that is not intended by the original depositor

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L432-L440

Vulnerability details

Impact

A malicious user can be granted more voting power than was intended by the original depositor.

Proof of Concept

In alterDelegateeOnBehalf(), the deposit owner can grant a signature to another user to give him the rights to alter delegatee based on his intent. We will show why the current implementation is flawed. Note that this vulnerability is based on the assumption that Alice did not grant another signature.

Importantly, the signature hash in alterDelegateeOnBehalf() is missing the old depositor delegatee.

Alice stakes and owns a deposit. Eve is the current delegatee of the deposit. She signs a signature to grant Bob the rights to alter that deposit's delegatee to himself. Bob, instead of using the signature, decides to sit on it. At this point, the old delegatee Eve still retains the voting power. Some time has passed, Alice wants to change delegatee to another user, Carol. Alice calls alterDelegatee() and change the delegatee from Eve to Carol. Carol is Alice's trusted partner, and hence decides to increase the stake by 10x in this deposit using stakeMore(), granting Carol more voting power. An important Uniswap governance proposal is announced and needs to be voted on. Bob calls alterDelegateeOnBehalf() now and change delegatee to himself right before the snapshot is taken for this proposal. Bob now acquires 10x the voting power Alice originally intended to grant him, and can influence the Uniswap governance proposal using this "stolen" voting power.

This is only possible because the signature hash does not care about the old delegatee, and only the new one. This means that regardless of how many times delegatee has been changed by the original depositor, it can always be changed back to Bob, the signature's new delegatee. Furthermore, the signature hash does not have any deadline. Bob can sit on the signature forever as long as Alice does not grant another user a signature.

Note that while the other OnBehalf() functions such as alterBeneficiaryOnBehalf() does not keep track of the old beneficiary as well, their impact are not so severe. It is not advantageous for a malicious beneficiary to sit on the signature, since the rewards are slowly accured to them only once they become beneficiary.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding deposit.delegatee as part of the signature. This way, we are narrowing the strength of the signature, such that it can only change a delegatee from A -> B, instead of ANY -> B.

Assessed type

Other

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Insufficient proof

[c4-judge]

MarioPoneder removed the grade

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as duplicate of #69

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder marked the issue as duplicate of #205

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-c

[yixxas]

This issue is incorrectly duped to #205 and should be deduped. The crux of this issue is the lack of the old delegatee in the signature. The described exploit scenario is only possible due to missing the old delegatee AND deadline in the signature. I agree with the sponsor that the lack of deadline in the signature itself does not present any serious issues for the protocol.

In this case however, a malicious old delegatee can effectively steal voting power from the deposit original owner in a way that he has not intended. The impact here is severe and clear.

In the described example in this report, I have presented one of many plausible scenarios where a malicious delegatee can abuse the signature granted to him beyond what is intended by the original depositor.

The only condition of this report is that the signature has not been intentionally revoked by the creator by making another signature to others or himself. The original depositor does not and should not have to be aware of the flaw in the signature he provided or even knowing the intimate details of using signature.

And because using alterDelegatee() and stakeMore() does not change or invalidate the old signature, and that these set of actions are perfectly reasonable ones for the original depositor to make, the presented scenario in the report is likely to happen to some users at some point.

Importantly, in the perspective of the original depositor, calling alterDelegatee() to change delegatee to a new one should remain at that new delegatee perpetually without any further actions from the original depositor. An old signature previously issued should not be able to alter the delegatee to a new one.

This issue meets the bar for medium severity, as impact of theft of voting power is severe, but on the condition that original depositor does not issue another signature and then later on change delegatee by alterDelegatee(). This is a reasonable assumption as it is perfectly within the set of actions that protocol users can and will make.

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[MarioPoneder]

Thank you for your comment and raising your concerns!

I understand that you do not agree with the duplication as this report just mentions but does not focus on the missing deadline problem.
The present issue is only possible if Alice doesn't create a new signature that's used by someone else in the meantime (or herself to cancel the signature).
Furthermore, voting power is not stolen but just delegated to someone Alice initially wanted to delegate it to and can alter the delegation anytime at will, because Alice is the depositor.
Due to the limited temporary impacts and low likelihood of this case, QA would be most appropriate. However, this report still boils down to the core issue that "signatures can be held indefinitely", therefore re-duplication to 205 is justified.

[c4-judge]

MarioPoneder marked the issue as duplicate of #205

[c4-judge]

MarioPoneder marked the issue as satisfactory

[yixxas]

Hi @MarioPoneder, I am writing this response as you seem to have misunderstood the attack described in this report. But I apologise in advance if this is out of line.

Based on your comments,

Furthermore, voting power is not stolen but just delegated to someone Alice initially wanted to delegate it to and can alter the delegation anytime at will, because Alice is the depositor.

Alice can alter delegation anytime at will because she is the depositor, but it is precisely the fact that it does not matter how many times Alice changes the delegatee that is the vulnerability described in this report. The attacker only needs to be the delegatee in one single moment to steal voting power, and that is right before the Uniswap snapshot (for context, Uniswap takes a snapshot of all UNI token holders on a certain block to determine voting power and the amount of voting power is frozen at this point for a particular proposal). This effect is neither temporary nor limited.

This is clearly described in this report.

Some time has passed, Alice wants to change delegatee to another user, Carol. Alice calls alterDelegatee() and change the delegatee from Eve to Carol. Carol is Alice's trusted partner, and hence decides to increase the stake by 10x in this deposit using stakeMore(), granting Carol more voting power. An important Uniswap governance proposal is announced and needs to be voted on. Bob calls alterDelegateeOnBehalf() now and change delegatee to himself right before the snapshot is taken for this proposal. Bob now acquires 10x the voting power Alice originally intended to grant him ...

This attack is still possible even with deadline added to the signature. But it does significantly limited the attack surface as the malicious attacker now has limited time to execute the attack.