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.

[Gua00va]

Hey, is this issue still open. I would like to work on it. Can you assign it to me?

[Eren-Yeaager]

Can you please assign this issue to me??

[pcaversaccio]

@Eren-Yeaager the issue is already resolved (pending merge) via PR: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4038.

[Eren-Yeaager]

ok

On Fri, Feb 24, 2023 at 8:35 PM Francisco @.***> wrote:

Closed #4035 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4035 as completed via #4038 https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4038.

— Reply to this email directly, view it on GitHub https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4035#event-8602835532, 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.