search

sense-finance / point-tokenization-vault

6 stars 0 forks source link

M-2: Rumpel admin will not be able to redeem rewards from external protocols on behalf of Rumpel wallets in some cases #28

Open ethanbennett opened 3 months ago

ethanbennett commented 3 months ago

Note: this finding has been updated to reflect the developments that occurred in subsequent discussions and research, and it now reflects the final state of the issue.

Severity: Medium risk

Context: RumpelModule.sol#L45

Description: If an external protocol attempts to verify a Safe signature by normal means (i.e., by passing a signature into the verification function), Rumpel admin will not be able to provide a valid signature for Rumpel wallets. The Rumpel wallet will treat the signature as a concatenated series of owners' signatures, and it will check that each signer is actually an owner of the Safe. The Rumpel admin is not an owner, so their signature will not be accepted.

Despite lacking a path for exploitation, this vulnerability represents a substantial issue in the event that a points-earning protocol requires a user signature to redeem points. If the protocol cannot redeem points on behalf of Rumpel wallets, the associated pTokens will be unbacked, and may cause the protocl to become insolvent.

Recommendation: Rumpel should create its own version of the compatibilityFallbackHandler, which retains all the functionality of the original, but implements isValidSignature in such a way that it will only reference the Safe's signedMessages mapping when validating signatures. This way, any signature registered by way of the signMessage function — which is available to Rumpel admin via the Rumpel module — will return the valid ERC-1271 magic value and be treated as valid by external protocols.

Additionally, it would be best to make this functionality upgradeable, as there is some degree of uncertainty around how external protocols will implement their points redemption mechanisms.

contract SignatureValidationBeacon is Ownable {
    address public signatureValidationContract;

    constructor(address validationImplementation) {
        owner = msg.sender;
        signatureValidationContract = validationImplementation;
    }

    function updateImplementation(address newImplementation) public onlyOwner {
        signatureValidationContract = newImplementation;
    }
}

contract SignatureValidationImplementation {
    function isValidSignature(
        bytes memory _messageData,
        bytes32 _messageHash,
        bytes memory _signature,
        Safe _safe
    ) public view returns (bool) {
        // Always check storage for signed messages; never checkSignatures
        require(safe.signedMessages(messageHash) != 0, "Hash not approved");
        return true;
    }
}

contract CompatibilityFallbackHandler {
    // ...

    // Note: it would also be best to avoid hard-coding this address
    SignatureValidationBeacon validationBeacon = 0x123;

    // Non-standard signature that matches expected Safe v1.4.1 ABI
    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, _data);
        bytes32 messageHash = keccak256(messageData);

        address implementation = validationBeacon.signatureValidationContract();

        require(
            SignatureValidationImplementation(implementation).isValidSignature(
                messageData,
                messageHash,
                _signature,
                safe
            )
        );

        return EIP1271_MAGIC_VALUE;
    }

    function isValidSignature(bytes32 _dataHash, bytes memory _signature) public view returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
        bytes32 messageHash = keccak256(messageData);

        address implementation = validationBeacon.signatureValidationContract();

        require(
            SignatureValidationImplementation(implementation).isValidSignature(
                messageData,
                messageHash,
                _signature,
                safe
            )
        );

        return UPDATED_MAGIC_VALUE;
    }

    // ...
}
ethanbennett commented 3 months ago

I updated this one to medium, since it seems to me at this point like the approach to validation covered in H-1 is the more likely one to be used in practice. Also, H-1 applies to both validation approaches. I'll continue thinking about these two though.

ethanbennett commented 2 months ago

Here are some notes capturing further research, and the reasons for likely recategorizing this one to "low."

ethanbennett commented 2 months ago

Note: some of the conclusions in this comment are corrected in subsequent comments

Got a response from the team and essentially confirmed my conclusions, with a little extra info. So to summarize those conclusions:

jparklev commented 2 months ago

Excellent research, sir. Ended up going with response 1 (changing the legacy function) here: https://github.com/sense-finance/rumpel-wallet/commit/0611c278559757c111babe3b6cd53ee6ede014dc How does that look?

We started on option 2 (removing the legacy function), but ran into an issue where the other isValidSignature is dependent on the legacy one. Lmk if we've missed something there – happy to make that change instead if so

ethanbennett commented 2 months ago

Great point @jparklev , I missed a step in my description/recommendation due to some confusion about which is really the legacy function: there is one which is marked as such (the one I linked to above), but then there's also an ERC-1271-compliant version which is only relevant when called by other Safes, and was removed from the compatibilityFallbackHandler in more recent commits. At some point, I started thinking of the wrong one as "legacy", since the non-legacy approach seems to have been effectively deprecated after v1.4.1. Just to attempt to make things a little more clear, here's where we're at so far:

The goal of this fix is basically to make sure that the Rumpel wallet (1) is compliant with ERC-1271 and (2) that it only ever references its signedMessages mapping to validate signatures. So your commit above is half of that picture, but the other half (which I neglected to mention above) would be changing the first parameter of your updated isValidSignature function to bytes32, since this is what is defined by ERC-1271.

However, this raises a small additional complication, because it would also require the removal of the existing isValidSignature(bytes32, bytes) function, plus changing the function signature of the existing entry-point that hits signedMessages (the other isValidSignature, L28). Removing the existing function is no issue, but the latter change opens the door to the possibility that external protocols expecting the non-standard isValidSignature function will not be able to validate a Rumpel wallet signature because it is now ERC-1271 compliant.

Because we don't have access to specific implementations that the Rumpel wallet will need to integrate with, I think it makes sense to account for the above possibility by duplicating the functionality in your updated version of isValidSignature: have one version with the compliant function signature, and one with the function signature that external developers may expect from the v1.4.1 Safe's implementation. That would look like this:

    // Non-standard signature that matches expected Safe v1.4.1 ABI
    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, _data);
        bytes32 messageHash = keccak256(messageData);

        /// @dev THIS IS THE CHANGE ----

        // Always check storage for signed messages; never checkSignatures
        require(safe.signedMessages(messageHash) != 0, "Hash not approved");

        /// @dev THIS IS THE CHANGE ----

        return EIP1271_MAGIC_VALUE;
    }

    // Standard ERC-1271 function signature
    function isValidSignature(bytes32 _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, _data);
        bytes32 messageHash = keccak256(messageData);

        /// @dev THIS IS THE CHANGE ----

        // Always check storage for signed messages; never checkSignatures
        require(safe.signedMessages(messageHash) != 0, "Hash not approved");

        /// @dev THIS IS THE CHANGE ----

        return EIP1271_MAGIC_VALUE;
    }

Normally it may be excessive (and may even defeat the purpose of standardizing this interface in ERC-1271) to account for these non-standard integrations. However, since the Safe is the clear leader in the smart contract wallet space, I think there's a reasonable chance that external protocol developers will look to its implementation when designing their own code.

Let me know if this makes sense!

jparklev commented 2 months ago

On the main branch, the non-legacy function, i.e. the ERC-1271-compliant function signature (bytes32) referred to in the first bullet point, is removed from the compatibilityFallbackHandler, and the "legacy" function referred to in the second bullet point has an updated, ERC-1271-compliant function signature (its first parameter being changed to bytes32). It is no longer just a legacy function.

Wow, now that's super confusing haha

One isValidSignature that complies with the function signature specified in ERC-1271 (with bytes32) and delegates the responsibility of validating the signature to msg.sender, which it assumes is also a Safe.

One possibly naive question: both versions of isValidSignature assume the caller will be a safe in v1.4.1, right? Since they both use msg.sender as such (even though one is wrapped in an ISignatureValidator interface). Basic assumption is like caller -> safe -> fallback -> check storage slot for the one, and caller -> safe -> fallback -> safe -> fallback -> check storage slot for the other. But feel free to set me straight lol

Let me know if this makes sense!

Yes, I believe so! With the one caveat in the question above. Certainly the point about Safe being the clear leader seems right.

Went ahead and made a change based on my understanding here – is this what you had in mind? https://github.com/sense-finance/rumpel-wallet/commit/d0f9a145ad8bbdc8436992284603ae14ac639534

ethanbennett commented 2 months ago

That’s not a naive question at all! This appears to be because it’s meant to be called by the Safe itself. This isn’t exactly how you would think you’d want it either, but ultimately, it requires yet another assumption about how external protocols may implement their functionality here.

On the subject of these assumptions, though, I realized that I could deduce a bit more about how standard signature verification looks in the front-end if I instead focused on simple login/TOC signatures, rather than trying to guess how they would be handled in a points-redemption scenario.

The TLDR of this research is that the wagmi NPM package (currently the standard for wallet connection in the space) will detect when the connecting account is a Safe, and instead of prompting a typical message signature, it will initiate a call to the Safe’s signMessage function. It also provides a verifyMessage function with the following interface, which seems to use an adapter (under the hood) to hit the isValidSignature functions:

import { verifyMessage } from '@wagmi/core'
import { config } from './config'

await verifyMessage(config, {
  address: '0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266',
  message: 'hello world',
  signature: '0x66edc32e2ab001213321ab7d959a2207fcef5190cc9abb6da5b0d2a8a9af2d4d2b0700e2c317c4106f337fd934fbbb0bf62efc8811a78603b33a8265d3b8f8cb1c',
})

In order to be compatible with libraries like this (and the Safe SDK), I think it’s best to not change the expected context of the signedMessages storage here (i.e., I think it's risky to remove msg.sender from the function call). However, the more uncertainty that I encounter around these interfaces you’ll need to integrate with, the more I think it also makes sense to make your custom isValidSignature functionality upgradeable by defining it in a separate contract.

You could create a simple “beacon” architecture for this: point your compatibilityFallbackHandler to a beacon contract that defines the signature validation implementation contract address, which itself can only be updated by the Rumpel admin. When anyone calls either implementation of isValidSignature, it will query the beacon for the current validation implementation address, then pass its parameters along to that implementation contract to handle the real validation logic. This way, if any unexpected implementations come up in the future, you have maximal flexibility to adjust how all Rumpel wallets validate signatures.

Here’s a pseudocode version of this implementation:

contract SignatureValidationBeacon is Ownable {
    address public signatureValidationContract;

    constructor(address validationImplementation) {
        owner = msg.sender;
        signatureValidationContract = validationImplementation;
    }

    function updateImplementation(address newImplementation) public onlyOwner {
        signatureValidationContract = newImplementation;
    }
}

contract SignatureValidationImplementation {
    function isValidSignature(
        bytes memory _messageData,
        bytes32 _messageHash,
        bytes memory _signature,
        Safe _safe
    ) public view returns (bool) {
        // Always check storage for signed messages; never checkSignatures
        require(safe.signedMessages(messageHash) != 0, "Hash not approved");
        return true;
    }
}

contract CompatibilityFallbackHandler {
    // ...

    SignatureValidationBeacon validationBeacon = 0x123;

    // Non-standard signature that matches expected Safe v1.4.1 ABI
    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, _data);
        bytes32 messageHash = keccak256(messageData);

        address implementation = validationBeacon.signatureValidationContract();

        require(
            SignatureValidationImplementation(implementation).isValidSignature(
                messageData,
                messageHash,
                _signature,
                safe
            )
        );

        return EIP1271_MAGIC_VALUE;
    }

    function isValidSignature(bytes32 _dataHash, bytes memory _signature) public view returns (bytes4) {
        // Caller should be a Safe
        Safe safe = Safe(payable(msg.sender));
        bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
        bytes32 messageHash = keccak256(messageData);

        address implementation = validationBeacon.signatureValidationContract();

        require(
            SignatureValidationImplementation(implementation).isValidSignature(
                messageData,
                messageHash,
                _signature,
                safe
            )
        );

        return UPDATED_MAGIC_VALUE;
    }

    // ...
}

Note that you may need to hardcode the beacon address in your compatibilityFallbackHandler, since it might be difficult to pass it a parameter on deployment (due to the proxy factory implementation). This is an antipattern, but in my opinion, is acceptable to enable this degree of flexibility for the future. I have not looked much into this though; it may be possible to pass it in via the proxy factory.

One more note: since it would in fact be the non-standard usage of wagmi that would hit the signedMessages mapping, I think it is actually fairly unlikely without this change that Rumpel admin could sign messages on behalf of the Safe. I would say that elevates this vulnerability back to a medium.

To review, this fix will:

  1. Preserve both interfaces of isValidSignature in v1.4.1 of the compatibilityFallbackHandler
  2. Reference the signedMessages mapping in all cases to enable non-owner Rumpel admins to create valid signatures regardless of how the external protocol attempts to validate them
  3. Make the signature validation logic for the Rumpel wallet upgradeable, in case any future integrations require something different
jparklev commented 2 months ago

since it would in fact be the non-standard usage of wagmi that would hit the signedMessages mapping, I think it is actually fairly unlikely without this change that Rumpel admin could sign messages on behalf of the Safe

Could you expand on this point a bit?

But overall, roger that. Would actually feel a ton of relief, making this specific piece upgradable now that you mention it. Reviewed those first two changes from the previous commits, and then how does this approach look to you WRT upgradability? https://github.com/sense-finance/rumpel-wallet/commit/e9f5c9627d50acb7e9cb0787df87e96cef8fbb0b

ethanbennett commented 2 months ago

since it would in fact be the non-standard usage of wagmi that would hit the signedMessages mapping, I think it is actually fairly unlikely without this change that Rumpel admin could sign messages on behalf of the Safe

Could you expand on this point a bit?

Sure, definitely could have been more clear here. Based on the docs for wagmi and the Safe SDK, I think the majority of frontend devs implementing this functionality will attempt to provide a real signature to validate (which would require the Rumpel admin to be an owner of the Safe in the off-the-shelf v1.4.1). Although both libraries allow for the signedMessages version of the validation, this is not a feature that is documented in either of them.

The "legacy" terminology threw me off before: in fact, neither mechanism for validating signatures is really a "legacy" function. The designation of a "legacy function" turned out to be a pretty misleading red herring, in the end.

But overall, roger that. Would actually feel a ton of relief, making this specific piece upgradable now that you mention it. Reviewed those first two changes from the previous commits, and then how does this approach look to you WRT upgradability? sense-finance/rumpel-wallet@e9f5c96

The changes look great! Better than the pseudocode — setting it directly in the compatibilityFallbackHandler is more efficient. With this change it's not technically a beacon proxy, if you care about the terminology, but that's obviously a minor note. And I totally agree, this feels way more robust now that this piece is upgradeable. I think we've finally got this one settled.