Use >=32 instead of ==32 in SignatureChecker for return data length check

[pcaversaccio]

In the ERC4626 function _tryGetAssetDecimals you use the following pattern with respect to the return data length check:

if (success && encodedDecimals.length >= 32) {
    uint256 returnedDecimals = abi.decode(encodedDecimals, (uint256));
    if (returnedDecimals <= type(uint8).max) {
        return (true, uint8(returnedDecimals));
    }
}

In the function isValidSignatureNow you use result.length == 32 instead for the return data length check. I think it would make sense due to consistency to also use result.length >= 32 in that case. FYI, I already discussed this issue with @frangio over Discord.

[HarshitSharma007]

@pcaversaccio can you please assign this issue to me.

[pcaversaccio]

I'm not part of the OZ team :-D. Cc: @Amxx

[frangio]

@HarshitSharma007 Feel free to submit a PR.

[Amxx]

I agree we should have consistency, but I wonder why it should be on >=32 and not on ==32

[frangio]

My reasoning for >= 32 is that abi.decode will accept data that is larger than needed, and we shouldn't make it more restrictive than the "native" Solidity decoding.

[Eren-Yeaager]

Can you please assign this issue to me??

[pcaversaccio]

@Eren-Yeaager the issue is already resolved (pending merge) via PR: #4038.

[Eren-Yeaager]

ok

…

On Fri, Feb 24, 2023 at 8:35 PM Francisco ***@***.***> wrote: Closed #4035 <#4035> as completed via #4038 <#4038>. — Reply to this email directly, view it on GitHub <#4035 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVXIKF3XKG4RGV4ED5MYW7LWZDE5PANCNFSM6AAAAAAUUS56ZI> . You are receiving this because you were mentioned.Message ID: <OpenZeppelin/openzeppelin-contracts/issue/4035/issue_event/8602835532@ github.com>

[yvonnezhangc]

@frangio @pcaversaccio could either of you help provide more context on this change?

ERC1271 specifies that the return value must be the bytes4 magic value 0x1626ba7e. Why is it a good idea to use >= rather than == here?

[pcaversaccio]

@frangio @pcaversaccio could either of you help provide more context on this change?

ERC1271 specifies that the return value must be the bytes4 magic value 0x1626ba7e. Why is it a good idea to use >= rather than == here?

See @frangio's argument here.