search

code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

`PositionManager.sol#permit()` fail to validate signatures from counterfactual wallets #156

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/base/PermitERC721.sol#L100

Vulnerability details

PositionManager.sol#permit() will fail to validate signatures from counterfactual wallets

Impact

This vulnerability impacts PositionManager.sol#premit() ability to validate signatures for counterfactual ERC-4337 accounts, limiting the usability for users of certain wallets that rely on AA.

Proof of Concept

EIP 1271 allows contracts to sign messages and works great in tandem with EIP 4337 (account abstraction).

As we know, in ERC-4337, the account is not deployed until the first UserOp is mined. Before then, the account exists "counterfactually" — it has an address even though it's not really deployed. Usually, this is great since we can use the counterfactual address to receive assets without deploying the account first.

However, if the user wants to sign a message for a permit for PositionManager, but his wallet currently exists "counterfactually" this becomes an issue since the PositionManger.pause is going to look at the user address, see that it has no code, and assume that the user is EOA, and reject the signature.

Since the contract isn't there yet, it wouldn't know to validate the staker signature through ERC-1271

File: ajna-core/src/base/PermitERC721.sol#L100-L110

        if (Address.isContract(owner)) {
            // bytes4(keccak256("isValidSignature(bytes32,bytes)") == 0x1626ba7e
            require(
                IERC1271(owner).isValidSignature(digest, abi.encodePacked(r_, s_, v_)) == 0x1626ba7e,
                "ajna/nft-unauthorized"
            );
        } else {
            address recoveredAddress = ecrecover(digest, v_, r_, s_);
            require(recoveredAddress != address(0), "ajna/nft-invalid-signature");
            require(recoveredAddress == owner, "ajna/nft-unauthorized");

This practically means PositionManager will fail to validate signatures for users of notorious wallets/projects, including Safe, Sequence, and Argent supporting ERC1271, but also ERC4337 wallets, even though they intend to support signatures by EIP1271 compliant wallets, as confirmed by these tests.

Given that the protocol decided to support contract-based wallets (that support ERC4337) and implement ERC1271, my assumption is that they "inherit" the possibility from the wallet providers to support ERC4337 too.

This issue may seem like a problem with ERC4337 or EIP1271. Still, it stems from the fact that protocol is taking responsibility to check the validity of signatures, but partially failing to trigger signature validation signatures for a group of wallets from a provider (the validation will succeed if the ERC4337 wallet is deployed).

Tools Used

Recommended Mitigation Steps

Thankfully, as of today, EIP-6492 solves this issue. The author of the EIP already implemented ERC-6492 in a universal library which can verify 6492, 712, and 1271 sigs with this pull request.

ERC6492 assumes that the signing contract will normally be a contract wallet, but it could be any contract that implements ERC-1271 and is deployed counterfactually.

That works because, as defined in the EIP, the wallet should return the magic value in both cases. If that doesn't work for you, I would advise clarifying in the docs and the contract itself that permit doesn't support counterfactual ERC4337 accounts.

Assessed type

Other

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor confirmed

Picodes commented 1 year ago

Unfortunately this is Out of scope for this contest: https://github.com/code-423n4/2023-05-ajna

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Out of scope