search

code-423n4 / 2022-02-aave-lens-findings

0 stars 0 forks source link

Approvals not cleared when transferring profile #22

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/modules/follow/ApprovalFollowModule.sol#L32

Vulnerability details

Impact

The ApprovalFollowModule.approve function is indexed by both (owner = IERC721(HUB).ownerOf(profileId), profileId) in case the profileId NFT is transferred. However, upon transfer, the old approvals are not cleared.

This can lead to similar issues as OpenSea not cancelling their sale offers upon NFT transfer. When the NFT is at some point transferred back to the original owner, all the old approvals are still intact which might not be expected by the owner.

Recommended Mitigation Steps

Consider resetting all approvals upon transfer.

Zer0dot commented 2 years ago

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

Zer0dot commented 2 years ago

Paging @oneski if you have any input

0xleastwood commented 2 years ago

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.

0xleastwood commented 2 years ago

Keeping this issue open and as is.

donosonaumczuk commented 2 years ago

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).

Zer0dot commented 2 years ago

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.

0xleastwood commented 2 years ago

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.

Zer0dot commented 2 years ago

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).