Platform: Code4rena
Start Date: 30/06/2023
Pot Size: $100,000 USDC
Total HM: 8
Participants: 22
Period: 14 days
Judge: Trust
Total Solo HM: 6
Id: 253
League: ETH
Rank: 3/22
Findings: 1
Award: $9,181.99
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: 0xc695
9181.9909 USDC - $9,181.99
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
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.
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.
The issue was identified through manual review of the contract mechanisms and their potential abuse, without the use of specific security tools.
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.
Rug-Pull
#0 - c4-pre-sort
2023-07-15T05:57:07Z
minhquanym marked the issue as primary issue
#1 - c4-sponsor
2023-07-18T13:27:22Z
CJ42 marked the issue as sponsor confirmed
#2 - CJ42
2023-07-18T13:28:55Z
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)
#3 - trust1995
2023-08-02T11:32:14Z
I believe rug pull vectors should be capped at M severity. This is a really important find though.
#4 - c4-judge
2023-08-02T11:32:21Z
trust1995 changed the severity to 2 (Med Risk)
#5 - c4-judge
2023-08-02T11:32:27Z
trust1995 marked the issue as satisfactory
#6 - c4-judge
2023-08-02T11:58:51Z
trust1995 marked the issue as selected for report
#7 - trust1995
2023-08-04T13:53:50Z
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.
#8 - MiloTruck
2023-08-04T15:08:57Z
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:
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!
#9 - skimaharvey
2023-08-04T15:30:53Z
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
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.
#10 - trust1995
2023-08-04T17:09:50Z
@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:
Therefore I decided to round up a weak med / strong analysis as sponsor has confirmed the finding.