IllIllI - Anyone can lock the unclaimed rewards of a migrating user

[sherlock-admin]

IllIllI

medium

Anyone can lock the unclaimed rewards of a migrating user

Summary

The function for claiming rewards does not have access control, allowing an attacker to send funds to an address no longer under the control of the original recipient.

Vulnerability Detail

The TrufVesting contract, which manages the vesting of token ownership, has a function whose specific task is to allow users to migrate everything to a new address if they lose their private key. The function migrates a user's vesting schedule and escrow locks to a new address, but does not migrate the user's earned rewards (a separate bug). Once a user is in need of the function being called, it is not correct for more funds to be sent to that address. However, because the internal VirtualStakingRewards contract doesn't have any access control on its public getReward() function, anyone can call that function with the old address, doing just that.

An attacker can call the function either after the migration happens (because they monitor the blockchain for migrations), or before the migration occurs (the attacker overheard the victim asking a coworker for help on what to do now that they've lost their private key).

Even if there is no migration happening, someone can call getReward(), causing that person to receive funds at an inopportune time (e.g. tax implications).

Impact

Rewards that the user has already earned but has not yet claimed become locked in an address they no longer control, rather than being migratable or rescuable. Fixing the migration of unclaimed rewards in migrateUser() does not fix this bug.

This POC shows Alice attacking after the migration, but the attack can happen before the migration too:

diff --git a/truflation-contracts/test/token/VotingEscrowTruf.t.sol b/truflation-contracts/test/token/VotingEscrowTruf.t.sol
index bfdf771..3deabf8 100644
--- a/truflation-contracts/test/token/VotingEscrowTruf.t.sol
+++ b/truflation-contracts/test/token/VotingEscrowTruf.t.sol
@@ -531,6 +531,49 @@ contract VotingEscrowTrufTest is Test {
         _validateLockup(bob, 0, amount, duration, ends, points, true);
     }

+    function testMigrateVestingLockRewards() external {
+        console.log("Migrate vesting lock and its rewards to new user");
+
+        uint256 amount = 100e18;
+        uint256 duration = 30 days;
+        assertEq(trufToken.balanceOf(carol), 0, "Initial balance should be zero");
+        _stakeVesting(amount, duration, carol);
+
+        // distribute some rewards, and let some accrue
+        vm.startPrank(owner);
+        trufToken.transfer(address(trufStakingRewards), amount);
+        trufStakingRewards.notifyRewardAmount(amount);
+        vm.stopPrank();
+        vm.warp(block.timestamp + 10 days);
+
+        // migrate the lock
+        vm.startPrank(vesting);
+        veTRUF.migrateVestingLock(carol, bob, 0);
+        vm.stopPrank();
+
+        uint256 rewards = trufStakingRewards.rewards(carol);
+        assertEq(rewards, 0, "Old address shouldn't have any rewards");
+
+        // Alice was watching, and locks the rewards at the old address
+        vm.prank(alice);
+        trufStakingRewards.getReward(carol);
+        assertEq(trufToken.balanceOf(carol), 0, "Rewards should be transferred to new user");
+
+        /**
+        [FAIL. Reason: assertion failed] testMigrateVestingLockRewards() (gas: 646896)
+        Logs:
+          Migrate vesting lock and its rewards to new user
+          Error: Old address shouldn't have any rewards
+          Error: a == b not satisfied [uint]
+                Left: 299999999999999375997
+               Right: 0
+          Error: Rewards should be transferred to new user
+          Error: a == b not satisfied [uint]
+                Left: 299999999999999375997
+               Right: 0
+         */
+    }
+
     function testMigrateVestingLockFailures() external {
         _stakeVesting(100e18, 30 days, alice);
         _stake(100e18, 30 days, alice, alice);

Code Snippet

Unlike the other functions, there is no onlyOperator modifier, and the user acted upon is not required to be msg.sender:

// File: src/staking/VirtualStakingRewards.sol : VirtualStakingRewards.getReward()   #1

126 @>     function getReward(address user) public updateReward(user) returns (uint256 reward) {
127            reward = rewards[user];
128            if (reward != 0) {
129                rewards[user] = 0;
130 @>             IERC20(rewardsToken).safeTransfer(user, reward);
131                emit RewardPaid(user, reward);
132            }
133:       }

https://github.com/sherlock-audit/2023-12-truflation/blob/main/truflation-contracts/src/staking/VirtualStakingRewards.sol#L126-L133

Tool used

Manual Review

Recommendation

Add the onlyOperator modifier, so the only way to get rewards is via VotingEscrowTruf.claimReward()

[IllIllI000]

@ryuheimat do you agree that this is a separate issue from #80?:

or before the migration occurs (the attacker overheard the victim asking a coworker for help on what to do now that they've lost their private key).

Even if there is no migration happening, someone can call getReward(), causing that person to receive funds at an inopportune time (e.g. tax implications).

specifically, that fixing the migration of rewards does not prevent someone from calling getReward() prior to a migration

[nevillehuang]

@IllIllI000 How would another user know that some user has already lost their private key other than speculation? The only way I could think of is:

• Front-running migrateUser() which is not possible on Base
• Speculating that a user has lost his private key as mentioned in this issue then attempting to call it maliciously hoping it locks the reward

TLDR any issue that mentions getReward locking can perhaps even be argued as invalid, given one can see it as the user calling getRewards doing the other user a favor to claim rewards by wasting gas.

[IllIllI000]

If an admin submits a migration on L1, because they don't want to use or can't use the L2 for whatever reason, it can be front-run.

[nevillehuang]

@IllIllI000 Got it, then this could possibly be valid. However this is assuming how admin will submit transactions so I will have to loop on @Czar102 to comment.

If I’m not wrong @Czar102 stance is if there is no information in the contest details, you cannot assume how the admin perform an action (such as submitting a transaction)

[Czar102]

but does not migrate the user's earned rewards (a separate bug)

Does it refer to vested tokens or other rewards? Are vested tokens migrated as well?

[IllIllI000]

It refers to the protocol's own in-scope token as a reward from the in-scope reward contract (does not refer to vested tokens, which are a separate pool of the protocol's own token). Vested tokens are properly migrated. The 'separate bug' is the one I filed as https://github.com/sherlock-audit/2023-12-truflation-judging/issues/125

[ryuheimat]

@IllIllI000 @nevillehuang This is duplicated issue of https://github.com/sherlock-audit/2023-12-truflation-judging/issues/80

[nevillehuang]

I got an internal response from base developer that it is no possible to front-run from a tx submitted from base chain, so closing this issue.

If you submit a transaction on Base, it's not possible to see the transaction until it is already onchain because the mempool is private; therefore, it is not possible to frontrun it on the network in that aspect.

[IllIllI000]

@nevillehuang The question wasn't about front-running on Base, it's about submitting the txn via the L1, which does not go through the Base mempool

[nevillehuang]

@IllIllI000 That was the dev response when I asked him the following question.

Oh i see then am i right to say if i call a function from any contracts deployed on base, tx is automatically submitted via an L2 sequencer, hence front running is not possible?

[nevillehuang]

Escalate

During my discussions with LSW @IllIllI000, I cannot confirm if front-running is possible on base chain. I am in ongoing discussions with a base chain dev on this, but escalating for further discussions.

[IllIllI000]

Optimism docs say you can send messages on L1 in order to call contracts on L2: https://docs.optimism.io/stack/protocol/deposit-flow#l1-processing it's done via L1CrossDomainMessenger.sendMessage(). Base has the same contract https://docs.base.org/base-contracts#l1-contract-addresses and you can see the same function being called every couple weeks: https://etherscan.io/address/0x866E82a600A1414e583f7F13623F1aC5d58b0Afa This is the proxy's implementation contract, and it has comments indicating the same purpose: https://etherscan.io/address/0x81c4bd600793ebd1c0323604e1f455fe50a951f8#code#F4#L249 . If the admin submits a migration via L1, it can be front-run. One use case of an admin choosing to do this, is when they wish to make atomic changes on both L1 and L2.

[sherlock-admin2]

Escalate

During my discussions with LSW @IllIllI000, I cannot confirm if front-running is possible on base chain. I am in ongoing discussions with a base chain dev on this, but escalating for further discussions.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

There is no claim reward for old account in current implement of migrate. So this issue is not valid in this audit. It's future fix for #80.

[IllIllI000]

Whether there is a fix for #80 or not, the tokens can be locked by an attacker, vs just sitting in the contract

[securitygrid]

1. If #80 is not confirmed as a valid issue, it means that the protocol does not care about users who have lost their private keys. Then, there is no point in considering reward at all.
2. If #80 is a valid issue, then this report is the fix for #80. This is a future issue which is out of scope.

[IllIllI000]

As I've said multiple times now, fixing #80 does not fix this issue

[ryuheimat]

@IllIllI000 fixing https://github.com/sherlock-audit/2023-12-truflation-judging/issues/80 requires to add onlyOperator modifier to getRewards function, so it could prevent front-running

[IllIllI000]

adding the modifier is not a required part of the fix for #80, since the bug being fixed is migrating user's rewards. Adding the modifier is a separate thing you're doing on top in order to fix this issue

[ryuheimat]

getReward function is permissionless and it sends rewards to allocated user. When migrating rewards of old user, we cannot claim rewards to new account, since it was allocated to old user. So we needed to add to param in getReward function, and add onlyOperator contract. When migrating rewards, we need to set to as new account.

[IllIllI000]

there are many ways to fix an issue, and the way you happen to fix an issue does not affect the validity of the bugs prior to the fix - it's the root cause's intention that matters, and I don't think the intention was to allow an attacker to lock rewards to the old address

[spacegliderrrr]

despite the conversation by watson and dev team:

• issue requires admin sending the function from an L1 bridge to L2, instead of just on L2. Makes 0 sense, watson knows this will never happen. Gas fees will make this unnecessarily expensive.
• rescuing this funds is the same as trying to rescue the funds inside the wallet

[IllIllI000]

One use case of an admin choosing to do this, is when they wish to make atomic changes on both L1 and L2. The bug is there, whether you think the admin is likely to do it or not

[spacegliderrrr]

yeah, a grief vulnerability (no profit motive for attacker) combined with the unlikely external condition to send from L1. (in the even more unlikely situation users lose their private keys and admin migrates them)

low at best

[IllIllI000]

Unmigrated rewards may be many multiples of the original locked amount, since you get a portion of the total reward pool based on your shares, not the raw amount invested. A ton of locked unmigrated votes, so it's easier for the attacker to pass a proposal, is plenty of an incentive for someone to want to do this attack

[nevillehuang]

@IllIllI000 This is the final response by base chain dev:

Hello, no, this is for bridge transactions; not specific transactions interacting with a dapp (e.g. Uniswap on Base) for the purposes of front-running.

so I am inclined to believe this is invalid

[IllIllI000]

@nevillehuang Can you ask them what access control on who is allowed to call the sendMessage() function there is? When I checked the caller, of some of the etherscan txns, it was random EOAs. Allowing an attacker to front-run being 'not the intended purpose' is not something that would prevent an attacker.

[IllIllI000]

For example, if you look at this from address, it's clearly a bot making cross-chain trades: https://etherscan.io/tx/0x5de00ac971333b4c5bf7cf3bf8d955a8459d46ec99c3f7a3f6219bacf02b5086

[nevillehuang]

@IllIllI000 I think this issue is invalid since it is not meant for the functionality to be called via a cross chain message. The protocol is meant to be used on base chain only

Think I might have mentioned this to you somewhere can’t remember.

[IllIllI000]

I don't think it says anywhere that the admin is not allowed to do it. The readme says, Trfulation Token, vesting, ve token, virtual staking rewards contract will be deployed on Base Mainnet, which does not exclude using Base's L1 contracts to interact with those contracts. Furthermore, there are in-scope contracts listed as to be deployed to eth mainnet. If you'd rather I get someone else to escalate this issue, I can do that, just let me know.

[nevillehuang]

@IllIllI000 while I believe this is invalid, I won’t be removing the escalation. I believe it will serve as a good discussion for future issues on the mechanisms of base chain

[detectiveking123]

Front-running admin transactions on ETH doesn't seem like it should be valid, ever. The admin should be trusted to use Flashbots or another private transaction stream.

If the admin was frontrun, clearly they made a mistake. But the admin is trusted to not make mistakes.

[nevillehuang]

To add on to @detectiveking123 arguments, this was previously explicitly mentioned by @Czar102 given flashbots can POSSIBLY be directly taken into account for validating such issues if transactions are submitted by admin. Though this involved direct locking of funds while the other is a griefing vector, I still stand by my previous argument that it is not meant for the functionality to be called via a cross chain message. The protocol is meant to be used on base chain only

[IllIllI000]

The linked comment talks about the fact that the 1 year DOS can be bypassed once the DOS has started, using flashbots. In this case, if the admin doesn't think to use flashbots the one time, the tokens are locked forever

[Czar102]

The linked comment explains that there is an easy workaround to repetitive griefing by frontrunning. I don't think it's fair to assume that all transactions are submitted via a private mempool unless it is explicitly mentioned in the README/docs.

I would like @IllIllI000 to show that it is possible to execute the transactions on Base before the sequencer. Isn't the sequencer whitelisted and pushing a message from L1 asks the sequencer to include it before forcing it?

If frontrunning on Base is not possible, I believe that while losing unclaimed rewards because of the migration being problematic is a medium (as if core functionality was impacted), taking into account that the attacker would need to frontrun the transaction and the wouldn't make a profit, this is a borderline med/low, now if it's not possible to see the tx to frontrun it before it is finalized (assuming the sequencer works as intended), I think this can be considered a low severity issue.

[IllIllI000]

Above, I link to the optimisim docs (Base is built using optimism's bedrock release) and there it says:

L1 processing:
1. An L1 entity, either a smart contract or an externally owned account (EOA), sends a deposit transaction to L1CrossDomainMessenger, using sendMessage
2. The L1 cross domain messenger calls its own _send function
3. _sendMessage calls the portal's depositTransaction function
4. The depositTransaction function runs a few sanity checks, and then emits a TransactionDeposited event.

L2 processing:
1. The op-node component looks for TransactionDeposited events on L1. If it sees any such events, it parses them.
2. Next, op-node converts those TransactionDeposited events into deposit transactions
3. n most cases user deposit transactions call the relayMessage function of L2CrossDomainMessenger
4. relayMessage runs a few sanity checks and then, if everything is good, calls the real target contract with the relayed calldata.

In what I'm outlining is:

1. The admin does L1.1 to initiate a transaction on L2, from L1 for calling migrateVestingLock(). The admin submits the txn and it's in the Ethereum L1 mempool, not yet mined.
2. The attacker sees the txn in the Ethereum L1 mempool, still unmined, and submits a txn calling getRewards() call on Ethereum L1 using the same method as the admin, but with a higher fee (or using flashbots with a high kickback).
3. The attacker's txn gets added to the next mined block
4. The admin's txn gets added either later in the same block, or in a later block
5. L2.1 processing takes over, and since the TransactionDeposited event from the attacker's txn appears first, it gets executed first, and the admin's txn gets front-run

The optimism docs explicitly state that the sequencer can be bypassed:

Mitigation
Users can always bypass the Sequencer by sending L2 transactions directly to the OptimismPortal contract on L1. Refer to the [Bypassing the Sequencer](https://docs.optimism.io/stack/protocol/outages#bypassing-the-sequencer) section for more information about this functionality.

Bypassing the Sequencer
A core security goal of OP Stack chains is that the Sequencer should not be able to prevent users from submitting transactions to the L2 chain. Users of OP Stack chains can always bypass the sequencer and include transactions in the L2 chain by sending their L2 transactions directly to the OptimismPortal contract on L1.

https://docs.optimism.io/stack/protocol/outages#mitigation

Note that my argument does not rely on the sequencer having an outage - the above states that users can always bypass the sequencer, by using the L1 contracts.

Please let me know if you're asking for some other information

[securitygrid]

The owner calls the function directly on Base. Why does he choose to call it from L1 to L2? This is making impossible assumptions.

[IllIllI000]

The owner calls the function directly on Base. Why does he choose to call it from L1 to L2? This is making impossible assumptions.

See: https://github.com/sherlock-audit/2023-12-truflation-judging/issues/124#issuecomment-1895646945

[securitygrid]

Impossible. don't argue any more.

[Czar102]

From the Optimism docs, execution time of the forced tx will be roughly within max_sequencer_drift of the transaction being submitted on L1 (if the sequencer determines sets of transactions for blocks at the respective timestamps), so it will be executed after the admin transaction is confirmed and relayed by the Sequencer. Hence, you can't frontrun such a transaction.

tl;dr: while you can force a tx to be executed, the Sequencer determines the ordering if it's working properly; and it will choose to execute the forced transaction after the original one

See the docs for more details.

[IllIllI000]

@Czar102 as I have listed above, both transactions (the attacker's and the admin's) are submitted to the L1 mempool. Nothing gets picked up in ethereum until it's mined - that's the guarantee of the blockchain. Whichever gets mined first is literally the definition of what happened first, even if they were submitted to the mempool in a different order. There is no txn block.timestamp, until there's a block where the txn has been mined.

[Czar102]

I believe you are misunderstanding how the rollup works. What the attacker can do is send an intent of an execution of the transaction, which needs to be executed by the Sequencer within a certain time period. The Sequencer submits a transaction finalizing the state changes. These are two different interactions and it doesn't matter which one is submitted earlier on L1, assuming both of them are executed within reasonable timeframes.

I misunderstood what situation the Watson was talking about. I don't think we will consider scenarios where the admin pushes transactions through L1 and gets frontran because of that.

[detectiveking123]

@Czar102 can you explain further about why you think Flashbots should not be assumed for admin transactions? Not using Flashbots is clearly an admin mistake imo.

[IllIllI000]

@Czar102 That new rule will take effect after this contest, correct? I believe it will have large consequences in terms of protocol expectations as to what is covered and what is not.

[MLON33]

For reference: https://github.com/sherlock-audit/2023-12-truflation-judging/issues/80#issuecomment-1889194362

https://github.com/truflation/truflation-contracts/pull/10

[ryuheimat]

https://github.com/truflation/truflation-contracts/pull/10

This PR was made to fix https://github.com/sherlock-audit/2023-12-truflation-judging/issues/80 We added onlyOperator modifier to getReward function, so it would fix this issue as well.

[Czar102]

That new rule will take effect after this contest, correct? I believe it will have large consequences in terms of protocol expectations as to what is covered and what is not.

There is no new rule. It has been clear that the team intends to interact with smart contracts through the Base network, so it's not a valid issue.

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: rejected

[IllIllI000]

I'll just note that nowhere in the readme does it say they will not use the L1, and they did in fact fix this bug