code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol#L252-L254 https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol#L316-L321

Vulnerability details

Impact

In the LSP0ERC725AccountCore contract, the execute and executeBatch functions use the LSP20CallVerification standard. In some cases, you may need to use verifyCallAfter. However, because an external call is made between verifyCall and verifyCallAfter, this can lead to potential issues if the call interacts with a contract that has permissions over the Universal Profile (UP) and follows a logic where it first transfers and then accepts ownership.

The LSP20 standard intends for the verifyAfterCall to be performed on the owner of the UP, but if the ownership changes in the interim due to the external call, the verification process may not be accurate. This could cause incorrect permissions to be granted, impacting the security of the contract and potentially enabling unauthorized actions.

Proof of Concept

Consider a scenario where the execute or executeBatch function calls an external contract that has permissions over the UP. The external contract, upon receiving the call, first transfers the ownership of the UP to a new owner and then accepts the ownership. In this case, when the execute or executeBatch function proceeds to verifyCallAfter, it verifies the original owner, not considering the change in ownership made by the external contract.

Tools Used

The issue was identified through manual review of the contract mechanisms and their potential misuse, without the use of specific security tools.

Recommended Mitigation Steps

-Restrict the ability to change ownership in the middle of function execution: Consider implementing a mechanism to prevent ownership transfer during the execution of certain sensitive functions. This could be achieved by introducing a lock state variable that prevents ownership changes when set, and is only unset at the conclusion of such functions.