stealth of funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/bd49f57c32a522563fc42feeee23c83c8b373405/contracts/LSP0ERC725Account/LSP0ERC725Account.sol#L41 https://github.com/code-423n4/2023-06-lukso/blob/bd49f57c32a522563fc42feeee23c83c8b373405/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol#L225 https://github.com/code-423n4/2023-06-lukso/blob/bd49f57c32a522563fc42feeee23c83c8b373405/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol#L283

Vulnerability details

[CRITICAL]

Impact

The LSP0ERC725Account contract executes calls to specified targets (provided in the arguments), the contract can receive native coins using the payable functions or directly transfered since the contract implements a receive function. However, the way the contract handles the execute function allows any user to steal all available native coins inside the contract.

Proof of Concept

consider the following scenario.

1. The LSP0ERC725Account contract has a balance of 100 ETH
2. A malicious user calls the execute() function with a target to which the address of a receiver contract ownend by the malicious user is assigned and a value argument equals LSP0ERC725Account.balance() and a msg.value = 0.
3. The LSP0ERC725Account proceeds to call the attacker's receiver contract fallback() function with value specified in the arguments which in our case msg.value equals the value specified in the args as the ERC725x specifies (NOTE the msg.value sent by the attacker is 0 however the one sent by the contract is the one provided in the arguments).
4. Since the fallback() function is payable it will transfer all available funds inside the LSP0ERC725Account contract to the attacker's receiver contract

   Tools Used

   manual analysis

   Recommended Mitigation Steps

   it is recommended to use the following code instead on line 244 inside LSP0ERC725Account

   bytes memory result = ERC725XCore._execute(
           operationType,
           target,
           msg.value,
           data
       );

   and not take it from the arguments as the current execute() function does

Assessed type

call/delegatecall

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Seems invalid

[CJ42]

This is intended behaviour.

• The msg.value is what is funded to the contract at runtime when calling the function.
• The value parameter is what is transferred, and come from the contract's balance (address(this).balance).

The contract can have received funds before via receive() function.

[c4-sponsor]

CJ42 marked the issue as sponsor disputed

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid