code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol#L279-L324

Vulnerability details

Bug Description

ERC725XCore's _execute() function allows four types of operations:

CALL for normal calls
DELEGATECALL
CREATE/CREATE2 for contract deployment
STATICCALL

The _executeBatch() function simply calls _execute() in a loop to perform multiple calls in a single transaction:

ERC725XCore.sol#L146-L153

        for (uint256 i = 0; i < operationsType.length; ) {
            result[i] = _execute(operationsType[i], targets[i], values[i], datas[i]);

            // Increment the iterator in unchecked block to save gas
            unchecked {
                ++i;
            }
        }

In LSP0AccountCore.sol, executeBatch() calls ERC725XCore._executeBatch() without performing any kind of checks on what calls are being made.

This is dangerous as a user could batch a call or contract deployment that sends LYX and a delegate call together.

If the delegate call happens to call a function that is payable, msg.value in this delegate call will be the same as the normal call, leading to unintended msg.value reuse. Furthermore, if the function called using delegate call does outgoing transfers of funds, the user could potentially drain his own LSP0 account by accident.

Impact

By batching a call/contract deployment that has msg.value and a delegate call togehter with executeBatch(), a user could unintentionally drain his own LSP0 account through reused msg.value in the delegate call.

Given that LSP0 accounts are meant to be used by regular users and the effects of batching a delegate call with other calls is not immediately obvious, this is a risk that should be mitigated to protect users.

Recommended Mitigation

In executeBatch(), ensure that msg.value is 0 when one of the batched calls is a delegate call. This would prevent msg.value from being reused across multiple calls.