Universal Data Key Permissions May Be Abused During Ownership Transfers

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L132-L137 https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L282-L288 https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L462

Vulnerability details

Impact

In LSP6KeyManager, when fetching permissions. we are looking for universal permissions (independent from the owner). If a UP owner transfers ownership to a new owner that uses a key manager, the previously set permissions (like access for a lot controller) remain intact. This can potentially enable the old owner to retain significant control over the UP, which could be abused to destabilize the contract or cause financial harm to the new owner and other participants.

Proof of Concept

The problem arises from the inability of the smart contract to identify and manage data keys that were set by previous owners. A malicious actor could set certain permissions while they are the owner of the UP, then transfer the ownership to a new owner. The old permissions would remain in effect, allowing the old owner to maintain undue control and possibly 'rug pull' or cause other harmful actions at a later date.

Tools Used

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

Recommended Mitigation Steps

To prevent potential abuse through residual permissions, the data keys for permissions should be made owner-specific. The following mitigation steps can be implemented:

• Hash the permission with the owner's address: When setting permissions, they can be hashed with the owner's address. This way, the permissions are specifically associated with a particular owner and do not affect subsequent owners.

• Add a nonce upon ownership transfer: To further ensure the uniqueness and irrelevance of old permissions, a random nonce can be added each time the ownership is transferred. This nonce can be used in conjunction with the address of the LSP6 when retrieving permissions, making any permissions set by old owners irrelevant.

Assessed type

Rug-Pull

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

CJ42 marked the issue as sponsor confirmed

[CJ42]

We are aware of the issue, but we want upgradability. Therefore we are trying to think of solutions that we already have in mind (e.g: using unique salts while hashing the AddressPermissions:Permissions:<controller> + salt prefix)

[trust1995]

I believe rug pull vectors should be capped at M severity. This is a really important find though.

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[trust1995]

Going to share additional rationale for severity assessment: Owner permissions vs Universal permissions are completely different concepts in LUKSO account abstraction. A user that receives "owner" permissions on an account should not be assuming permissions are reset - it is not stated at any point. The issue at hand is that at the smart contract-level it is impossible to know if some user has permissions set (This can still be checked on blockchain indexers / etherscan). This points to a somewhat problematic design, which is why I've decided to award it with Medium severity.

The conditionals and the unfounded trust required from the receiver for any damage to occur make the finding not eligible for High severity.

[MiloTruck]

Hi @trust1995, I don't mean to question your judgement here, but I don't really understand why this issue is considered a medium severity bug.

A user that receives "owner" permissions on an account should not be assuming permissions are reset - it is not stated at any point.

Wouldn't this mean that any impact resulting from a previous owner maliciously configuring permissions is a user mistake (as it is the responsibility of the current owner to ensure permissions are correct), making the issue QA?

Additionally, in my opinion, the likelihood of such a scenario occurring is extremely low since LSP0 accounts, unlike NFTs, aren't meant to be traded and should not be transferred between untrusted parties in the first place.

The issue at hand is that at the smart contract-level it is impossible to know if some user has permissions set (This can still be checked on blockchain indexers / etherscan). This points to a somewhat problematic design, which is why I've decided to award it with Medium severity.

Shouldn't design issues fall under QA according to C4's severity rules? While it is true that this cannot be checked on-chain, there are ways to side-step this issue by monitoring what permissions are added off-chain.

Would also like to highlight that there are many other ways for a previous owner to maliciously configure the LSP0 account without permissions that cannot be checked at a smart contract level:

• Previous owner can add an extension to transfer tokens out from the account.
• Previous owner can approve other addresses controlled by himself to transfer LSP7/LSP8 tokens on behalf of the LSP0 account by calling authorizeOperator() through execute().

This suggests that the sponsor was aware of the risks related to the transferring ownership of a LSP0 account, and deemed them as acceptable for the current design.

Would like to hear your opinion to understand from a judge's POV, thanks!

[skimaharvey]

Going to share additional rationale for severity assessment: Owner permissions vs Universal permissions are completely different concepts in LUKSO account abstraction. A user that receives "owner" permissions on an account should not be assuming permissions are reset - it is not stated at any point. The issue at hand is that at the smart contract-level it is impossible to know if some user has permissions set (This can still be checked on blockchain indexers / etherscan). This points to a somewhat problematic design, which is why I've decided to award it with Medium severity.

The conditionals and the unfounded trust required from the receiver for any damage to occur make the finding not eligible for High severity.

Agreed with everything said here. As stated, the main issue is that they are not retrievable from the smart contract directly which is not ideal. We intend to fix it by

• enforcing that the permissions are retrievable at the SC level
• and/or when there is a change in owner old permissions should easily be discarded (through for example adding a salt set at the Key Manager level)

We are still thinking about it and are open to any good ideas you might have. but yes 'High' severity seems exaggerated as it is your duty as a new owner to not blindly trust and make sure that old permissions do not remain through an indexer for example.

and yes @MiloTruck it is true for controllers or extensions. Anything that could have permissions on the UP.

[trust1995]

@MiloTruck good points raised. In an ideal world this would be part of the architecture section of a top analysis report. However, we're still not there yet. I've factored in these considerations:

1. The submission was eye-opening for the team
2. Functionality is lacking (can't view permissions trustlessly).
3. From chats with the team the usage of account transfers is not as unlikely as initially seems.

Therefore I decided to round up a weak med / strong analysis as sponsor has confirmed the finding.