search

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

0 stars 0 forks source link

Double collection + Refrain users from collecting publication #35

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/InteractionLogic.sol#L97

Vulnerability details

Impact

  1. By mistake user can collect same publication twice costing user monetary losses (collect fees)

  2. Attacker can exhaust collect limit (LimitedTimedFeeCollectModule.sol) which is set on a publication by collecting same publication again and again until collect limit is exhausted. This gives unfair chance to other users

  3. User can set a collect limit and collect his own publication near to collect limit. Other users will think to buy this fast selling publication

Proof of Concept

Case 1:

  1. User A creates a publication
  2. User B collect the publication by giving appropriate fees
  3. User B mistakenly collect the same publication and give fees. Contract gives no error and user is provided by another collect nft which does not make sense

Case 2:

  1. User A creates a publication with collect limit 25
  2. User A himself collect his own publication 20 times
  3. Other User sees that User A publication is depleting fast so they collect the publication and falls into the trap of fake collection

Recommended Mitigation Steps

If user has already collected a publication then collect function should revert

require (IERC721(collectNFT).balanceOf(collector) =0, "Already collected the publication")
oneski commented 2 years ago

Declined, this is by design. Alternative collect modules can include additional logic to prevent this behavior.

0xleastwood commented 2 years ago

I think I can mostly agree that it doesn't seem likely a user would call collect() more than once (it can be easily side-stepped by using multiple EOAs). However, is there any reason why this isn't enforced in this specific module? Is it meant to act as a reference implementation to be built upon? @oneski

Zer0dot commented 2 years ago

I think I can mostly agree that it doesn't seem likely a user would call collect() more than once (it can be easily side-stepped by using multiple EOAs). However, is there any reason why this isn't enforced in this specific module? Is it meant to act as a reference implementation to be built upon? @oneski

So the idea is that the fee is the anti-spam mechanism here. If a user wants to pay for 10 collects, they should be allowed to. Plus, as you mentioned limiting on a per-wallet basis isn't really ideal!

0xleastwood commented 2 years ago

I agree with the sponsor, not really an easy fix to this and I think alternative logic can be added down the line.