search

code-423n4 / 2023-07-lens-findings

0 stars 0 forks source link

Inconsistent encoding of arrays in `MetaTxLib` #142

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L143-L153 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L100-L109

Vulnerability details

Bug Description

According to the EIP-712 specification, arrays are encoded by concatenating its elements and passing the result to keccak256:

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

An example of a correct implementation can be seen in validateUnfollowSignature(), where the idsOfProfilesToUnfollow array is passed to keccak256 after using abi.encodePacked():

MetaTxLib.sol#L368

                        keccak256(abi.encodePacked(idsOfProfilesToUnfollow)),

However, some other functions in MetaTxLib encode arrays differently, which differs from the EIP-712 specification.

Some functions do not encode the array at all, and pass the array to abi.encode() alongside other arguments in its struct. For example, in validatePostSignature(), the postParams.actionModules array is not encoded by itself:

MetaTxLib.sol#L143-L153

                    abi.encode(
                        Typehash.POST,
                        postParams.profileId,
                        keccak256(bytes(postParams.contentURI)),
                        postParams.actionModules,
                        _hashActionModulesInitDatas(postParams.actionModulesInitDatas),
                        postParams.referenceModule,
                        keccak256(postParams.referenceModuleInitData),
                        _getAndIncrementNonce(signature.signer),
                        signature.deadline
                    )

Other instances of this include:

Secondly, the validateChangeDelegatedExecutorsConfigSignature() function encodes the delegatedExecutors and approvals arrays using abi.encodePacked(), but do not pass it to keccak256:

MetaTxLib.sol#L100-L109

                    abi.encode(
                        Typehash.CHANGE_DELEGATED_EXECUTORS_CONFIG,
                        delegatorProfileId,
                        abi.encodePacked(delegatedExecutors),
                        abi.encodePacked(approvals),
                        configNumber,
                        switchToGivenConfig,
                        nonce,
                        deadline
                    )

Impact

As arrays are encoded incorrectly, the signature verification in the functions listed above is not EIP-712 compliant.

Contracts or dapps/backends that encode arrays according to the rules specified in EIP-712 will end up with different signatures, causing any of the functions listed above to revert when called.

Moreover, the inconsistent encoding of arrays might be extremely confusing to developers who wish to use these functions to implement meta-transactions.

Recommended Mitigation

Consider encoding arrays correctly in the functions listed above, which can be achieved by calling abi.encodePacked() on the array and passing its results to keccak256.

Assessed type

Other

donosonaumczuk commented 1 year ago

We confirm it but we think this should be Low instead.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as disagree with severity

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

Picodes commented 1 year ago

Following the same reasoning as in https://github.com/code-423n4/2023-07-lens-findings/issues/141, I'll keep Medium severity here as EIP compliance is of great importance for integrators and compatibility, so I consider this an instance of " function of the protocol [is] impacted", the function being the EIP712 compliance.

c4-judge commented 1 year ago

Picodes marked the issue as primary issue