Lens Protocol V2 - evmboi32's results

Judge has assessed an item in Issue #55 as 2 risk. The relevant finding follows:

If we take a look at the EIP712 standard https://eips.ethereum.org/EIPS/eip-712 it states the following

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

There are a few examples in the MetaTxLib.sol that don’t follow this convenction.

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L103-L104

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L147

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L242-L243

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L245

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L272-L273

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L275

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L297-L298

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L437-L438

According to the EIP712 standard arrays should be encoded as: keccak256(abi.encode(array)).

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/FollowNFT.sol#L480-L520

Vulnerability details

Impact

A user can perform a self-follow during a migration process. For this proof of concept, a user uses two accounts. Let's call them A and B.

Proof of Concept

• User follows profile B from profile A and gets the followTokenId with id x
• User transfers the followTokenId with id x from profile A to profile B (this needs to be done to pass the 4th if check)
• Then he calls the batchMigrateFollows(profileBId, profileBId, x) on the hub contract, where x is the id of the followTokenId that he now owns.

This will invoke the tryMigrate function on the FollowNFT.

function tryMigrate(
    uint256 followerProfileId,
    address followerProfileOwner,
    uint256 idOfProfileFollowed,
    uint256 followTokenId
) external onlyHub returns (uint48) {
    // Migrated FollowNFTs should have `originalFollowTimestamp` set
    if (_followDataByFollowTokenId[followTokenId].originalFollowTimestamp != 0) {
        return 0; // Already migrated
    }

    if (_followedProfileId != idOfProfileFollowed) {
        revert Errors.InvalidParameter();
    }

    if (!_exists(followTokenId)) {
        return 0; // Doesn't exist
    }

    address followTokenOwner = ownerOf(followTokenId);

    // ProfileNFT and FollowNFT should be in the same account
    if (followerProfileOwner != followTokenOwner) {
        return 0; // Not holding both Profile & Follow NFTs together
    }

    unchecked {
        ++_followerCount;
    }

    _followTokenIdByFollowerProfileId[followerProfileId] = followTokenId;

    uint48 mintTimestamp = uint48(StorageLib.getTokenData(followTokenId).mintTimestamp);

    _followDataByFollowTokenId[followTokenId].followerProfileId = uint160(followerProfileId);
    _followDataByFollowTokenId[followTokenId].originalFollowTimestamp = mintTimestamp;
    _followDataByFollowTokenId[followTokenId].followTimestamp = mintTimestamp;

    super._burn(followTokenId);
    return mintTimestamp;
}

The function above doesn't check if the user is performing a selfFollow which should be forbidden. All the checks pass and the user successfully performs a selfFollow.

As he owns the profile and hence the followNFT the state now reads that the follower at followTokenId x is the owner himself.

Tools Used

VS Code

Recommended Mitigation Steps

Add a check in the tryMigrate function that checks if the two users are the same.

require(followerProfileId != idOfProfileFollowed, "ERR: Self follow");

Assessed type

Invalid Validation

StateSet emits the event twice

The setState function emits the StateSet event twice. Once in the internal call of the _setState function, and another time in the setState function. Should emit it only once.

The following code is inside the GovernanceLib.sol
https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/GovernanceLib.sol#L50-L69

function setState(Types.ProtocolState newState) external {
    // NOTE: This does not follow the CEI-pattern, but there is no interaction and this allows to abstract `_setState` logic.
    Types.ProtocolState prevState = _setState(newState);
    // If the sender is the emergency admin, prevent them from reducing restrictions.
    if (msg.sender == StorageLib.getEmergencyAdmin()) {
        if (newState <= prevState) {
            revert Errors.EmergencyAdminCanOnlyPauseFurther();
        }
    } else if (msg.sender != StorageLib.getGovernance()) {
        revert Errors.NotGovernanceOrEmergencyAdmin();
    }
    // @audit emits the StateSet event twice, once in `_setState` and once here.
    emit Events.StateSet(msg.sender, prevState, newState, block.timestamp);
}

function _setState(Types.ProtocolState newState) private returns (Types.ProtocolState) {
    Types.ProtocolState prevState = StorageLib.getState();
    StorageLib.setState(newState);
    emit Events.StateSet(msg.sender, prevState, newState, block.timestamp);
    return prevState;
}

Not following the EIP712 standard correctly

If we take a look at the EIP712 standard https://eips.ethereum.org/EIPS/eip-712 it states the following

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

There are a few examples in the MetaTxLib.sol that don't follow this convenction.

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L103-L104

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L147

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L242-L243

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L245

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L272-L273

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L275

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L297-L298

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/libraries/MetaTxLib.sol#L437-L438

According to the EIP712 standard arrays should be encoded as: keccak256(abi.encode(array)).