Aave Lens contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

User will pay lesser Collect Module fees and the difference will be borne by Publication owner. This way Publication owner will always be on loss

Attacker can also make this a business in which he can collect victim nft at discounted price and then sell that NFT to other users using transfer function

Proof of Concept

1. User A create a new post publication using createPost in PublishingLogic.sol#L122

2. User B wants to collect the post publication by User A. He call collect function on InteractionLogic.sol#L97

3. Collect function will call processCollect on Collect Module and asks User B for fees X (Treasury fees + User A fees)

4. User B does not approve and transaction fails

5. Now to reduce this fees X, User B simply creates a mirror for User A publication using createMirror function at PublishingLogic.sol#L247

6. A new mirror gets created pointing to User A publication and owned by User B

7. User B simply call collect function on InteractionLogic.sol#L97

8. This time again processCollect on Collect Module is ran but since the call is coming from mirror, a referral fees is calculated for mirror owner

9. In our case mirror owner is User B so final fees becomes ((Treasury fees + User B referral fees + (User A fees -User B referral fees) )

10. So In this case User B gets a discount of User B referral fees and User A incur that lose

Recommended Mitigation Steps

User should not be allowed to collect using a mirror owned by his own profile. Although there is a bypass in which User can create mirror to victim publication from one wallet and user other wallet to make the collect

8 Low issue identified as depicted below:

Zero address checks missing on setting Governance

1. While setting Governance via _setGovernance on LensHub.sol#L850-L854, there is no check to see if new Governance is address(0)
2. This can be disastrous as multiple functions like whitelisting profile/module etc will become unusable

Remediation: Change the _setGovernance like below:

function _setGovernance(address newGovernance) internal {
require(newGovernance!=address(0), "Incorrect Governance");
        address prevGovernance = _governance;
        _governance = newGovernance;
        emit Events.GovernanceSet(msg.sender, prevGovernance, newGovernance, block.timestamp);
    }

Old users are not impacted on changing Follow module

1. User A creates a new Profile using createProfile at LensHub.sol#L142 and has set address(0) as vars.followModule. A new profile gets created
2. User B follows User A and is issued a NFT from User A. Since followModule for User A was address(0) so no checks were performed and no fees was involved
3. User A decides to change the follow Module using setFollowModule at PublishingLogic.sol#L80 and makes this to FeeFollowModule.sol
4. Now if anyone wants to follow User A they will need to pay fees.
5. However Any old user who were already following User A (without any fees) will not be asked for this fees and would remain following User A for eg User B

Remediation: If followModule is changed using setFollowModule then new followModule should run on existing followers

Publication/Comment/Mirror can be collected for deleted profile

1. User A decides to burn his profile using burn function of LensHub.sol#L493. User A expects all data to be deleted
2. User B can still collect publication, comment, mirror belonging to User A even though User A does not exist in the system. User A will still receive payment from collect module
3. Correct checks are implemented for Follow NFT which checks whether profile exists or not

Remediation: Similar to follow function, implement checks in comment, publication, mirror creation to see if target profile exists/active

Stack too deep checks missing

1. It was observed that the prevention applied for Stack too deep error was missing in _processCollect function of LimitedTimedFeeCollectModule.sol#L159. Even though the same prevention is applied in _processCollectWithReferral function at LimitedTimedFeeCollectModule.sol#L180-L188

Remediation: Change all collect module treasury fees calculation to below (Like change LimitedTimedFeeCollectModule.sol#L159 to below):

address treasury;
        uint256 treasuryAmount;

        // Avoids stack too deep
        {
            uint16 treasuryFee;
            (treasury, treasuryFee) = _treasuryData();
            treasuryAmount = (amount * treasuryFee) / BPS_MAX;
        }

Incorrect implementation of _setTreasuryFee

1. It was observed that if newTreasuryFee >= BPS_MAX / 2 then the tresaury fees will be rejected. However this also rejects if newTreasuryFee = BPS_MAX / 2 (50%) which should not be the case

Remediation: Change the formula to:

if (newTreasuryFee > BPS_MAX / 2) revert Errors.InitParamsInvalid();

Multiple NFT can have same symbol

1. The NFT symbol is derived by taking bytes4 on user handle in both follow and collect function of InteractionLogic.sol#L60-L67 (and L117-L128)
2. Assume User A has handle aaaaa and User B has handle aaaab
3. In this case both user will have same NFT symbol as first 4 bytes in both cases is "aaaa" (only 5th byte differ)

bytes4 firstBytes = bytes4(bytes(handle)); which means only first 4 bytes are considered which is "aaaa" in both cases

string memory followNFTSymbol = string(
                    abi.encodePacked(firstBytes, Constants.FOLLOW_NFT_SYMBOL_SUFFIX)
                );
				

4. This could confuse users

Remediation:

1. Generate a separate logic to aware users that 2 profiles or publications can have same NFT symbol or Create a separate logic to calculate NFT symbol

Incorrect License specified

1. It was observed that LensMultiState.sol was marked with license SPDX-License-Identifier: MIT. Although all remaining contracts are licensed under SPDX-License-Identifier: AGPL-3.0-only

Remediation: Remove MIT license and add AGPL-3.0-only. Modify LensMultiState.sol#L1 to below:

// SPDX-License-Identifier: AGPL-3.0-only 

Contract State should not change in Library

1. Ideally Library should perform only those operation which does not impact Contract state.
2. But all functions at PublishingLogic.sol & InteractionLogic.sol are changing the contract state (creating new profile/post, minting nft etc)

Remediation: Move all state changing functions to a contract

1. In _createComment function of LensHub.sol#L881, use argument ++_profileById[vars.profileId].pubCount in call to PublishingLogic.createComment and remove the Line LensHub.sol#L887 as _profileById[vars.profileId].pubCount++; is no more required now

2. In mint function of CollectNFT.sol#L51, there is no need to store incremented value of _tokenIdCounter in local variable. Instead remove CollectNFT.sol#L51 and modify CollectNFT.sol#L52 to _mint(to, ++_tokenIdCounter);

3. In mint function of FollowNFT.sol#L68, there is no need to store incremented value of _tokenIdCounter in local variable. Instead remove FollowNFT.sol#L68 and modify FollowNFT.sol#L69 to _mint(to, ++_tokenIdCounter);

4. In collect module contract like LimitedFeeCollectModule.sol, both _processCollectWithReferral and _processCollect can be removed from all the collect module and placed into a single library which all collect modules can utilize