073.md

csanuragjain

medium

User funds can be stolen

Summary

If the transaction on L2 fails then the (alias of msg.sender) is given the mint value on L2 instead of tx.origin. This could lead to one user stealing other user funds as shown in POC

Vulnerability Detail

1. Assume App A is using optimism

2. App A has one Contract C1 having a function called "vote"

3. The vote function simply accepts the ether and sends them to its L2 contract C2

4. Lets say User A deposits 5 ether while calling "vote" function

5. "vote" function calls "depositTransaction" function at OptimismPortal.sol

 function depositTransaction(
        address _to,
        uint256 _value,
        uint64 _gasLimit,
        bool _isCreation,
        bytes memory _data
    ) public payable metered(_gasLimit) {
	...
	address from = msg.sender;
        if (msg.sender != tx.origin) {
            from = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
        }
	...
	}

6. Since msg.sender is contract C1, so an alias C3 is used as from address

7. Now this transaction is ran by "state_transition.go"

func (st *StateTransition) TransitionDb() (*ExecutionResult, error) {
	if mint := st.msg.Mint(); mint != nil {
		st.state.AddBalance(st.msg.From(), mint)
	}
	snap := st.state.Snapshot()

	result, err := st.innerTransitionDb()
	// Failed deposits must still be included. Unless we cannot produce the block at all due to the gas limit.
	// On deposit failure, we rewind any state changes from after the minting, and increment the nonce.
	if err != nil && err != ErrGasLimitReached && st.msg.IsDepositTx() {
		st.state.RevertToSnapshot(snap)
		...
	}
	return result, err
}

8. In beginning contract C3 is minted the value passed by User A which is 5

st.state.AddBalance(st.msg.From(), mint)

9. After this snapshot of state is taken and then execution begins

snap := st.state.Snapshot()

	result, err := st.innerTransitionDb()

10. Lets say execution fails (say function was paused or precondition failed) so below code executes and state is reverted to snapshot

if err != nil && err != ErrGasLimitReached && st.msg.IsDepositTx() {
		st.state.RevertToSnapshot(snap)
		...
	}

11. Since minting is done before taking snapshot so Contract C3 is still minted amount 5

12. Now User B asks contract C1 for vote with amount 5 and passes 0 msg.value

13. Contract C1 calls depositTransaction with Mint as 0 and Value as 5

14. The state_transition.go checks whether C3(alias C1) has required balance, which is present due to failed transaction

15. Hence the call to Vote succeeds for User B even though no amount was provided by him

evm.Context.Transfer(evm.StateDB, caller.Address(), addr, value)

Impact

User will lose funds

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/op-geth/core/state_transition.go#L307

Tool used

Manual Review

Recommendation

In case of error, Instead of minting "from" address, "tx.origin" should have been minted. This will prevent other users from stealing victim balance For this the deposit event need to be changed to include tx.origin as well.