Attacker could take advantage of a Re-Org attack and steal users Voting Weight.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/UniStaker.sol#L256-L261
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/UniStaker.sol#L638-L659
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/UniStaker.sol#L605-L616

Vulnerability details

Impact

In the Stake function(), the surrogate contracts that holds a Stakers votingweight are deployed or created using the CREATE method where the address derivation depends only on the nonce.
Re-orgs can happen in all EVM chains. In Ethereum, where Unistaker is deployed, "it's not so common" but still happens as the last one happened not so long ago:
https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg
The following exploit can be exploited due to the use of the CREATE method, during the course of a Re-org.

Proof of Concept

Bob calls stake,
Alice sees the network Re-org and calls stake as well, Frontrunning Bob, thus creating a DelegationSurrogate contract with an address in which Bobs votingweight will be sent to and because the CREATE method is used in deploying the DelegationSurrogate contract, both of them could theoritically, stake with different params and deploy DelegationSurrogate at the same address.

if (address(_surrogate) == address(0)) {
      _surrogate = new DelegationSurrogate(STAKE_TOKEN, _delegatee);
      surrogates[_delegatee] = _surrogate;

All of Bob's votingweight will be transferred to this Surrogate contract, controlled by Alice as well (since Her votingweight is also sent to the same DelegationSurrogate contract).
Alice can then proceed to call alterDelegatee(), to change her Surrogate contract thereby transferring all the voting tokens (both hers and Bobs) to a new Surrogatecontract which causes a huge loss to Bob.

function alterDelegatee(DepositIdentifier _depositId, address _newDelegatee) external {
    Deposit storage deposit = deposits[_depositId];
    _revertIfNotDepositOwner(deposit, msg.sender);
    _alterDelegatee(deposit, _depositId, _newDelegatee);
  }

 function _alterDelegatee(
    Deposit storage deposit,
    DepositIdentifier _depositId,
    address _newDelegatee
  ) internal {
    _revertIfAddressZero(_newDelegatee);
    DelegationSurrogate _oldSurrogate = surrogates[deposit.delegatee];
    emit DelegateeAltered(_depositId, deposit.delegatee, _newDelegatee);
    deposit.delegatee = _newDelegatee;
    DelegationSurrogate _newSurrogate = _fetchOrDeploySurrogate(_newDelegatee);
    _stakeTokenSafeTransferFrom(address(_oldSurrogate), address(_newSurrogate), deposit.balance);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Deploy such contracts via CREATE2 method with salt that includes the msg.sender

Assessed type

Other

[MarioPoneder]

Insufficient proof how this could be exploited on unlikely reorg.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Insufficient proof

[shealtielanz]

Good day @MarioPoneder
The warden here is showing how Bob can loose his staked tokens (Voting tokens) that will be sent to a surrogate contact during a reorg

The issue pointed out here is that based on the CREATE method being used, in the event of a reorg, Bob stakes n amount of UNI tokens, Alice sees the network reorg and stakes n amount of UNI tokens too front running Bob,
both Alice and Bob will have their surrogate contract deploy at the same address and as such their N amount of staked tokens will then be sent to the same surrogate contract controlled by Alice.

Alice being the malicious actor will then call alterDelegatee() (in order to take full control of the tokens) and deploy a new surrogate contract which automatically transfers Bobs tokens + Her tokens from the old surrogate to the new surrogate contract, making it impossible for Bob to access His tokens. hereby stealing all of Bob’s stake.
I hope this explanation helps as this report contains sufficient proof of the attack imo

Reorgs still do happen on main net
https://ethereum.org/en/developers/docs/consensus-mechanisms/pos/attack-and-defense#attacks-by-small-stakeholders

Similar Past findings that have been given medium severity.
code-423n4/2023-08-pooltogether-findings#31
code-423n4/2023-01-rabbithole-findings#661
code-423n4/2023-04-frankencoin-findings#155

[Jnrlouis]

I think this is a valid medium issue @MarioPoneder as the warden showed how this issue could be exploited to its maximum.
They went ahead to show how the surrogate contract will eventually be changed as the attacker will make a call to alterdelegatee() after the reorg happens thereby stripping the victim of all their tokens and any potential chances of getting them back.
and Looking at previous reorg related reports, this has more proof.

[MarioPoneder]

Thanks you for your comments!

Again, there is insufficient proof how a user could be harmed. If Bob's staking transaction is rolled-back due to the re-org, his staking token transfer is rolled back too.
Alice can either can either front-run Bob (doesn't require re-org) or stake after the re-org in order to have a surrogate that has Bob's previous surrogate address.
However, Bob doesn't have a mapping entry surrogates[_delegatee] = _surrogate; for the corresponding surrogate in case of being front-run or a re-org. Therefore, Bob cannot interact with Alice's surrogate through the protocol as it's now mapped to Alice.

Consequently, leaving this as it is.

[shealtielanz]

Thanks for your feedback @MarioPoneder

but the bone of contention here is that in the case of a reorg,
the surrogate contract of the both parties will be deployed at the “same address”
and when this is done their delegatee address is set to the same surrogatecontract address based on the state update done in the mapping you highlighted.

the mapping you highlighted: surrogates[_delegatee] = _surrogate;
basically just maps the newly deployed “malicious surrogate contract” to a delegatee address.
Which in the case of a reorg, will
at first be Alice’s (attacker) preferred delegatee address that will be mapped to the surrogate contract (since they are the front runners.)
Then Bob preferred delegatee will also be mapped to this “same malicious surrogate contract” as well based on the update made to the mapping.
So It’s just the ”maliciously deployed surrogate” been set to both their delegate address which is very much possible in this context.
And this “malicious surrogate contract” is where the staked UNI tokens of both parties will be sent to.

Now Alice can call the alterDelegatee() function and change the surrogate contract transferring all staked tokens of both parties out.
Thank you!