`_hashActionModulesInitDatas()` doesn't encode `bytes` array according to EIP-712

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L160-L170

Vulnerability details

Bug Description

The _hashActionModulesInitDatas() function is used to encode the actionModulesInitDatas parameter when verifying signatures in accordance to EIP-712:

MetaTxLib.sol#L160-L170

    function _hashActionModulesInitDatas(bytes[] memory actionModulesInitDatas) private pure returns (bytes32) {
        bytes32[] memory actionModulesInitDatasHashes = new bytes32[](actionModulesInitDatas.length);
        uint256 i;
        while (i < actionModulesInitDatas.length) {
            actionModulesInitDatasHashes[i] = keccak256(abi.encode(actionModulesInitDatas[i]));
            unchecked {
                ++i;
            }
        }
        return keccak256(abi.encodePacked(actionModulesInitDatasHashes));
    }

As seen from above, it calls abi.encode() on every bytes element in the array before encoding the entire array. However, according to EIP-712, bytes should be passed to keccak256 directly, without abi.encode():

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

When abi.encode() is called on each bytes element, the offset and length of the bytes object will also be included in the data that is passed to keccak256. For example:

bytes memory b = "AAAA";

// abi.encode(b) will give:
[00]: 0000000000000000000000000000000000000000000000000000000000000020 // offset
[32]: 0000000000000000000000000000000000000000000000000000000000000004 // length
[64]: 4141414100000000000000000000000000000000000000000000000000000000 // contents

Therefore, the following functions do not comply with EIP-712 as they use _hashActionModulesInitDatas() when verifying signatures:

• validatePostSignature()
• validateCommentSignature()
• validateQuoteSignature()

A correct example of encoding a bytes array can be seen in validateFollowSignature(), where each element in the bytes array is passed to keccak256 directly.

Impact

Contracts or dapps/backends that encode actionModulesInitDatas 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.

Recommended Mitigation

In _hashActionModulesInitDatas(), pass actionModulesInitDatas[i] directly to keccak256 instead of using abi.encode():

MetaTxLib.sol#L160-L170

    function _hashActionModulesInitDatas(bytes[] memory actionModulesInitDatas) private pure returns (bytes32) {
        bytes32[] memory actionModulesInitDatasHashes = new bytes32[](actionModulesInitDatas.length);
        uint256 i;
        while (i < actionModulesInitDatas.length) {
-           actionModulesInitDatasHashes[i] = keccak256(abi.encode(actionModulesInitDatas[i]));
+           actionModulesInitDatasHashes[i] = keccak256(actionModulesInitDatas[i]);
            unchecked {
                ++i;
            }
        }
        return keccak256(abi.encodePacked(actionModulesInitDatasHashes));
    }

Assessed type

Other

[donosonaumczuk]

Similar to #142 - "assets are not at risk: function incorrect as to spec" then we think it should be Low severity

[c4-sponsor]

donosonaumczuk marked the issue as disagree with severity

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

Keeping duplicate as the general issue is the non compliance of array encoding

[c4-judge]

Picodes marked the issue as duplicate of #142

[MiloTruck]

Hi @Picodes,

I originally submitted this as a separate issue from #142 as it violates a different rule in EIP-712. In #142, the encoding of arrays are wrong, while in this issue, the encoding of arrays is technically correct; it's the bytes within it that are encoded wrongly.

Also, if you decide that this issue should still remain a duplicate, shouldn't this one be nullified since the main issue is mine as well? Not sure if it really matters for award calculation or the final report.

Thanks!

[Picodes]

@MiloTruck I understand why you originally submitted separate reports. You're totally right to highlight that without #142, this issue would still exist so it wouldn't comply with EIP-712.

My reasoning here is that the root cause is that the dev team didn't follow the EIP to encode these arrays, so correctly mitigating #142 only would very likely lead to this issue being fixed as they'll also fix this in the process. I therefore consider this as a "sub-issue" of #142. On the opposite for example your report #141 is treated separately as fixing #142 wouldn't have led to the team fixing it.

For the award formula: this is taken into account in the computation script!