sherlock-audit / 2024-07-sense-points-marketplace-judging

8 stars 5 forks source link

AuditorPraise - rumpel wallet may not work with signatures from standard ERC1271 wallets #78

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

AuditorPraise

Medium

rumpel wallet may not work with signatures from standard ERC1271 wallets

Summary

some versions of safe don't work with signatures from standard ERC12271 wallets

Vulnerability Detail

According to this comment in RumpelGuard.sol, Rumpel wallet will be compatible with Safe v1.3.0-libs.0

// @dev Compatible with Safe v1.3.0-libs.0, the last Safe Ethereum mainnet release, so it can't use module execution hooks.

If the safe version on which Rumpel wallet is built on is v1.3.0, rumpel wallet won't work with signatures from standard ERC1271 wallets and here's why:

v1.3.0 safe version defaults to the old MAGIC_VAULE when checking if the signature is correct:

https://github.com/safe-global/safe-smart-account/blob/v1.3.0/contracts/GnosisSafe.sol#L285

require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");

https://github.com/safe-global/safe-smart-account/blob/v1.3.0/contracts/interfaces/ISignatureValidator.sol

contract ISignatureValidatorConstants {
    // bytes4(keccak256("isValidSignature(bytes,bytes)")
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
}

As Per EIP-1271 standard ERC1271_MAGIC_VAULE should be 0x1626ba7e instead of 0x20c13b0b and function name should be isValidSignature(bytes32,bytes) instead of isValidSignature(bytes,bytes). Due to this, signature verifier contract go fallback function and return unexpected value and never return ERC1271_MAGIC_VALUE and always revert execTransaction function.

Impact

rumpel wallet may not work with signatures from standard ERC1271 wallets if it is built on Safe v1.3.0-libs.0

Code Snippet

https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/rumpel-wallet/src/RumpelGuard.sol#L10

Tool used

Manual Review

Recommendation

Don't use version v1.3.0, use Safe 1.5.0+

AuditorPraise commented 2 months ago

@WangSecurity @jparklev Hey, pls can i get a reason on why this was excluded? so that i can know if i should go ahead with my escalation