Handle

Dravee

Vulnerability details

Impact

msg.sender costs 2 gas (CALLER opcode) Storing in a memory variable costs 3 gas (MSTORE opcode) Reading from a memory variable costs 3 gas (MLOAD opcode)

msg.sender shouldn't get cached in a variable.

Proof of Concept

See the @audit-info tags in L2Migrator.sol:claimStake(), there are 7 occurences of the cached variable delegator instead of directly using the cheaper msg.sender.

File: L2Migrator.sol
262:         address delegator = msg.sender; //@audit-info : msg.sender shouldn't get cached
263:         bytes32 leaf = keccak256(
264:             abi.encodePacked(delegator, _delegate, _stake, _fees) //@audit-info : delegator is used instead of msg.sender
265:         );
266: 
267:         require(
268:             merkleSnapshot.verify(keccak256("LIP-73"), _proof, leaf),
269:             "L2Migrator#claimStake: INVALID_PROOF"
270:         );
271: 
272:         require(
273:             !migratedDelegators[delegator], //@audit-info : delegator is used instead of msg.sender
274:             "L2Migrator#claimStake: ALREADY_MIGRATED"
275:         );
276: 
277:         migratedDelegators[delegator] = true; //@audit-info : delegator is used instead of msg.sender
278:         claimedDelegatedStake[_delegate] += _stake;
279: 
280:         address pool = delegatorPools[_delegate];
281: 
282:         address delegate = _delegate;
283:         if (_newDelegate != address(0)) {
284:             delegate = _newDelegate;
285:         }
286: 
287:         if (pool != address(0)) {
288:             // Claim stake that is held by the delegator pool
289:             IDelegatorPool(pool).claim(delegator, _stake); //@audit-info : delegator is used instead of msg.sender
290:         } else {
291:             bondFor(_stake, delegator, delegate); //@audit-info : delegator is used instead of msg.sender
292:         }
293: 
294:         // Only EOAs are included in the snapshot so we do not need to worry about
295:         // the insufficeint gas stipend with transfer()
296:         if (_fees > 0) {
297:             payable(delegator).transfer(_fees); //@audit-info : delegator is used instead of msg.sender
298:         }
299: 
300:         emit StakeClaimed(delegator, delegate, _stake, _fees); //@audit-info : delegator is used instead of msg.sender
301:     }

Tools Used

VS Code

Recommended Mitigation Steps

As one might argue here, the gain in readability may not be worth the delegator variable instead of a simple comment. Use msg.sender directly instead of caching it in a variable.

code-423n4 / 2022-01-livepeer-findings