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

0 stars 0 forks source link

Inconsistent behavior in the *FeeCollectModule contract can cause DOS. #14

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/core/modules/collect/FeeCollectModule.sol#L139-L177

Vulnerability details

Impact

In the *FeeCollectModule contract, the recipient of the fee is specified by the user when the post is created, that is, even if the profileNFT is transferred or destroyed, the fee will still be sent to the address specified by the user when the post is created.

    function initializePublicationCollectModule(
        uint256 profileId,
        uint256 pubId,
        bytes calldata data
    ) external override onlyHub returns (bytes memory) {
        ...
        _dataByPublicationByProfile[profileId][pubId].recipient = recipient;

When collecting the mirror, the receiver of the referralFee is the owner of the referrer's profileNFT, that is, the referralFee will be sent to different addresses along with the transfer of the referrer's profileNFT.

        if (referralFee != 0) {
            uint256 referralAmount = (adjustedAmount * referralFee) / BPS_MAX;
            adjustedAmount = adjustedAmount - referralAmount;
            address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

            IERC20(currency).safeTransferFrom(collector, referralRecipient, referralAmount);
        }

When profileNFT is destroyed, require(owner != address(0) in the ownerOf function will fail, resulting in DOS.

    function _burn(uint256 tokenId) internal virtual {
        address owner = ERC721Time.ownerOf(tokenId);

        _beforeTokenTransfer(owner, address(0), tokenId);

        // Clear approvals
        _approve(address(0), tokenId);

        _balances[owner] -= 1;
        delete _tokenData[tokenId];

        emit Transfer(owner, address(0), tokenId);
    }
...
    function ownerOf(uint256 tokenId) public view virtual override returns (address) {
        address owner = _tokenData[tokenId].owner;
        require(owner != address(0), 'ERC721: owner query for nonexistent token');
        return owner;
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L365-L377

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L84-L88

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/FeeCollectModule.sol#L139-L177

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedFeeCollectModule.sol#L157-L195

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L168-L206

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/TimedFeeCollectModule.sol#L153-L191

Tools Used

None

Recommended Mitigation Steps

Specify referralRecipient when creating the mirror or check if the referrer's profileNFT is destroyed when collecting the mirror

Zer0dot commented 2 years ago

This is only an issue when a profile is deleted (burned), in which case UIs have multiple choices:

  1. Stop displaying all the burnt profile's publications
  2. Redirect users, when collecting mirrors, to the original publication
  3. Prevent all collects

I don't think this adds any risk to the protocol and although it's valid, we will not be taking any action.

Zer0dot commented 2 years ago

This is a duplicate of #67