Aave Lens contest - cmichel's results

Actually a good point!

Duplicate of #46

Declined. This is by design. Governance can allow contracts/addresses to mint. If governance allows a malicious actor that is the fault of governance. Governance can also allow contracts that implement auction/commit reveal or other functionality as well to manage the profile minting system.

The protocol should take no opinion on this by default.

While I agree that this is the fault of the governance, I think it still stands that any whitelisted user may be compromised or become malicious at a later point in time. For that reason, I think this issue has some validity although due to the unlikely nature (requiring a faulty governance), I'll mark this as medium risk for now.

While I agree that this is the fault of the governance, I think it still stands that any whitelisted user may be compromised or become malicious at a later point in time. For that reason, I think this issue has some validity although due to the unlikely nature (requiring a faulty governance), I'll mark this as medium risk for now.

I disagree on the medium risk. Governance being faulty is not enough of a reason IMO. Like most governance-included protocols, governance getting compromised bears risks that are largely unavoidable.

While I agree that a faulty governance is an extremely unlikely outcome. It isn't stated in the README and as per the judges guidelines, profiles can be front-run under specific stated assumptions/external requirements. A malicious whitelisted profile creator can reasonably front-run other creators.

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Fair enough, though we were aware of this-- it's clear it falls into a medium severity. We're acknowledging this!

Declined. This is by design. Governance can allow contracts/addresses to mint. If governance allows a malicious actor that is the fault of governance. Governance can also allow contracts that implement auction or other functionality as well to manage the profile minting system.

The protocol should take no opinion on this by default.

I will mark this as medium risk for the same reasons outlined in #26

Not sure if this makes sense, using your own referrer as a profile would basically be the equivalent of just having your publication collected directly without referral. In which case, what is the vulnerability?

I think they mean the case where Alice posts something and Bob wants to collect it. So Bob instead of just collecting directly from Alice publication, he creates a secondary profile, Bob2, mirrors publication through Bob2 and now Bob collects through Bob2's mirror, basically using the referral fee as a collecting discount for himself.

Even if we enforce referral profile owner to be different than collecting profile owner (aka comparing ownerOf(profileId) instead of profileId) people can just use different addresses for the purpose.

I saw this before but I assumed most of people will use the frontend, which will handle this automatically. But I believe that in the long term is possible that people learn the trick and, as rational agents, use it.

Awaiting for @Zer0dot thoughts on this.

I see, thanks @donosonaumczuk, at the end of the day this is a social protocol. Just like how on legacy social media, users can spam/abuse things, it's a given here that we will never have a fully foolproof system. Plus, with the chain history being visible, tools can (and I'd like to see them) be built to filter out nasty behavior. @oneski want to weigh in?

I think there is nothing more to do here

I think its naive to think that this will be properly handled by the front-end and won't be abused by users at some point in time. As such, I'm inclined to treat this as a valid medium risk issue because it fits the category of value leakage by the protocol. However, I can understand that there is no easy fix to this because it is impossible to link EOAs as originating from the same person.

Yeah I think that's fair, since we're dealing with a social protocol there's a degree of social "etiquette" expected from users. Since this does not harm the protocol or the original publication creator, I don't see it as a significant flaw. However, it does of course harm curators. I tend to disagree on the point about frontends @0xleastwood though, it seems to me that if this were to be a problem, it would be fairly straightforward to point fingers at those abusing the system.

Yeah I think that's fair, since we're dealing with a social protocol there's a degree of social "etiquette" expected from users. Since this does not harm the protocol or the original publication creator, I don't see it as a significant flaw. However, it does of course harm curators. I tend to disagree on the point about frontends @0xleastwood though, it seems to me that if this were to be a problem, it would be fairly straightforward to point fingers at those abusing the system.

While I understand an attacker would need to call functions directly to set this up, I'm not fully onboard with the idea that we could punish users who abuse the system in such a way. As per the judging guidelines, any issue that leaks value from the protocol is medium risk.

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I think its perfectly find if this issue is acknowledged and you guys have decided to handle it through front-end design. However, it does not take away from the fact that it is still somewhat of an exploit.

Fair enough! We can roll with medium.

This is known and acceptable, users should be able to check their approvals even if they don't have the profile.

Paging @oneski if you have any input

I think this issue is entirely valid, it should not be on the users to manage their approvals so much. It would be much safer to wipe approvals on transfers and avoid this issue altogether.

Keeping this issue open and as is.

I think this issue is entirely valid, it should not be on the users to manage their approvals so much. It would be much safer to wipe approvals on transfers and avoid this issue altogether.

I would say wiping on transfers is a bit annoying as I could be transfefring my profile between two addresses I own. It would be a better alternative to wipe on re-initialization by passing a boolean keepPreviousState flag (or something similar).

So after some more discussion, I think the points here are valid, but we were aware of this from the beginning. There are pros and cons to maintaining state. This module will not be present at launch and further iteration can modify it. Unfortunately there is no profile NFT transfer hook in follow modules currently. As this is still in my eyes not a direct vulnerability but more a caveat of how this specific system is designed, I would no longer dispute it but I would mark it as a low severity issue.

So after some more discussion, I think the points here are valid, but we were aware of this from the beginning. There are pros and cons to maintaining state. This module will not be present at launch and further iteration can modify it. Unfortunately there is no profile NFT transfer hook in follow modules currently. As this is still in my eyes not a direct vulnerability but more a caveat of how this specific system is designed, I would no longer dispute it but I would mark it as a low severity issue.

While I agree with you that giving users the ability to save the previous state on transfer makes sense and understand that the necessary changes to do this can be put in place in the future. I think its best I stay consistent with the judging rulebook as per below:

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I believe the functionality of the protocol is impacted in this scenario so I think its fair to keep this as medium risk.

Sounds fair, acknowledging. This is something we're going to look into deeper for newer or updated functionality (we won't be deploying this module yet).

Declined, this is by design.

Governance will decide what contracts are allowed to mint via the allowlist. If governance wishes to have a more centralized system, it will only approve contracts that have numerical caps within their code.

I agree with the sponsor, I think this can already be handled by the governance strictly approving contracts with numerical caps or limiting the allowlist of who can create a profile. As such, I'm inclined to mark this as invalid because the recommendation can already be implemented or adhered to by the governance.

In light of another issue, I will mark this as a valid issue because #66 outlines a similar concern. The two issues reference different parts of the codebase so I think its fair to keep them distinct. However, I'd normally like to see a bit more detail on how non-whitelisted users can benefit from an infinite minter.