AlterDelegatee signature is weak and can be misused

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L110

Vulnerability details

Impact

The implementation of the AlterDelegatee signature in the system is vulnerable due to its weak structure, potentially leading to undesired delegations of voting power. This weakness allows for manipulation that could affect governance proposal outcomes.

Proof of Concept

Consider the following scenario:

1. Alice stakes funds and decides to delegate her voting power to Bob for Governance proposal Agreements & Disclosures #1, creating an off-chain AlterDelegatee signature which she sends to Bob.
2. Bob retains the signature without executing alterDelegateeOnBehalf().
3. Alice aims to change her delegate to Charlie for an important Governance proposal High level access functions can create points of failure (Not captured by 4nalyzer)  #2 and uses alterDelegatee() to do so.
4. Right before the snapshot for voting power, Bob uses the previously issued signature to delegate Alice's voting power to himself for Governance proposal High level access functions can create points of failure (Not captured by 4nalyzer)  #2, against Alice's current intent.

This misuse occurs because the signature for AlterDelegatee does not specify a deadline or confirm the current delegatee, enabling unintended delegations.

Tools Used

Manual Review

Recommended Mitigation Steps

To enhance the security and specificity of the AlterDelegatee process, implement the following changes:

• Introduce a deadline parameter to the signature, mirroring the time-bound security found in permitAndXXX() methods. This ensures the signature cannot be used after a certain time, preventing delayed misuse.
• Include the oldDelegatee address in the signature. This addition requires that any delegation change must explicitly state the transition from the current delegatee to a new one, adding an extra layer of validation and preventing unauthorized changes.

Assessed type

Governance

[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-b

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as satisfactory

[marchev]

@MarioPoneder Can you please remove the grade-b label to avoid this issue mistakenly being treated as QA? This has previously happened in other contests. Thank you!