M-1: `RumpelGuard` cannot prevent users from directly redeeming points from external protocols in some cases

[ethanbennett]

Severity: Medium risk

Context: RumpelGuard.sol#L48

Description: In order for the Rumpel wallet to be flexible enough to interact with a wide range of external points-issuing protocols, it allows users to delegatecall to SignMessageLib.signMessage with an arbitrary message. This message is then marked as "signed" in the Safe's storage, and anyone can validate it by providing that same hashed message as an argument to the isValidSignature function.

This mechanism does not specify or implement any functionality related to the conveyance of this signed message to external protocols, and in fact, signatures like these are often sent to external protocols by way of HTTP requests. This is typically the case for logging in with an EOA, but more significantly, it was confirmed to be the likely design for some points-earning protocols' redemption mechanisms, which Rumpel will need to prevent users from accessing directly.

This is how message signing and verification would look with those protocols:

1. The protocol prompts the user to sign two messages: one to confirm ownership of the account, and another to agree to the terms of service. Let's say those messages are the simple strings "I own this account" and "I agree to the terms"
2. The user sends two delegatecall transactions to the SignMessageLib contract via the Safe's execTransaction function: one with the message "I own this account" and the other with "I agree to the terms".
3. The Safe checks with RumpelGuard before executing the transaction. checkTransaction allows the delegatecall to SignMessageLib, so the transaction proceeds
4. SignMessageLib executes its signMessage function in the context of the Safe. Unlike the process for EOA (externally-owned account) message signatures, this function simply hashes the user's input and then stores it in a mapping of hashed messages to integers (with the value 1)
5. After calling signMessage with both "I own this account" and "I agree to the terms", the user receives the hashes corresponding with each
6. The user now sends each of these hashes back to the external protocol by way of its HTTP API, outside the purview of the RumpelGuard
7. The external protocol validates the signature. In this scenario, let's assume they use the arguments (providedHash, emptyString) in order to intentionally hit the signedMessages mapping (the other possibility is covered in Finding M-2)
8. isValidSignature checks its mapping of signed messages for whether the value associated with the provided hashed message is 0. The values for both of the user's messages were set to 1 when they called signMessage, so the function returns the ERC-1271-specified magic value for each to certify to the protocol that the signatures are valid
9. With the authentication handshake complete, the external protocol will now send its reward tokens directly to the Rumpel wallet without requiring further action from the user

The impact of this vulnerability depends on the particular reward token. If, for instance, it was USDC — the prototypical example mentioned internally of a token that will always be transferrable by the user — the RumpelGuard could not prevent the user from transferring their rewards out of their Rumpel wallet. This user could then redeem their pTokens for more reward tokens in the Point Tokenization Vault, which would double their earnings at the expense of other Rumpel users and render the protocol insolvent.

If, however, the reward token is unique to the points-issuing protocol, its transfer functionality would need to be explicitly enabled by Rumpel admin in the RumpelGuard in order for the user to profit from this exploit. If these tokens become stuck in the Rumpel wallet, admin can easily transfer them out with the RumpelModule. But crucially, this would not be the case if the external protocol allows the user to define a secondary account to receive their reward tokens — a common enough design pattern to warrant considering the overall impact to be high-severity in more cases than not.

Recommendation: This is a difficult problem, because there is no established standard for the exact data that must be signed in any given context. As a result, it would not be easy to effectively modulate the user's permission to sign messages with their Rumpel wallet. And since signing messages is essential for logging into a great number of external protocols, fully blocking this functionality is also not a viable option.

Although it may be out of scope for the immediate response to this review, the most robust and ideal solution would likely leverage cryptoeconomic incentives to make it possible, but not beneficial, for a user to recover their rewards themselves. It could use similar mechanisms to handle the "bad debt" from unbacked pTokens when a user redeems their points directly from external protocols after minting pTokens, or the Point Tokenization Vault could enforce that all pToken redemptions are "Merkle-based" (ensuring that all pTokens are backed by points before allowing their redemption).

In lieu of such large changes to the protocol, the Rumpel wallet could implement its own version of SignMessageLib like the following:

// pseudocode: do not copy/paste directly
function signMessage(bytes calldata _data) external {
    bytes32 msgHash = getMessageHash(_data);
    require(_isMessageAllowed(_data), "Message not allowed");
    signedMessages[msgHash] = 1;
    emit SignMsg(msgHash);
}

function signMessageAdmin(bytes calldata _data) external {
    bytes32 msgHash = getMessageHash(_data);
    signedMessages[msgHash] = 1;
    emit SignMsg(msgHash);
}

function _isMessageAllowed(bytes calldata _data) internal view returns (bool) {
    for (uint256 i = 0; i < signatureDataAllowList.length; i++) {
        if (signatureMessageAllowList[i] == _data) return true;
    }
    return false;
}

This solution would implement an allow list for messages that can be signed by the user, which could be maintained by the admin and restricted such that it could only be updated through the RumpelModule. signMessageAdmin could be called by RumpelModule to allow admins the ability to sign messages without restrictions.

As mentioned above, however, the unpredictability of the required messages complicates this approach in the long term.

[jparklev]

Yes, thanks for your patience on this one. This and https://github.com/sense-finance/point-tokenization-vault/issues/28 are tricky. For this issue, we're actually slightly less concerned than might be expected (though still concerned) for two reasons:

1) We plan to "permanently allow" very sparingly, and accept that any token we do that for will never be a reward token redeemable by pTokens

2) If the token is allowed to transfer prior to release, we expect we'll need to disable it prior to distribution if there's any risk of something like this. We already have a situation where we expect to do this with ENA, where we need to enable transfers for it because users will want to add it to their wallet for the points boost. But because it's the reward token as well, we plan to freeze it a day before reward token release, claim, sweep, then unfreeze. In that case, even if the user is able to claim somehow, we can still sweep what's needed

That said, still the ideal situation is that we're working with the protocol teams, and we have a nice way to claim rewards for users, but, as you mention, a lot depends on these external protocols and we can't always rely on that.

There are still the unhappy paths for us 1) where we're caught unaware by a reward token release event, and the token is enabled for transfer, and the users can claim with their signature 2) if the external protocol allows the user to define a secondary account to receive their reward tokens (this was a good point)

So we're still thinking about those. For now, we plan to make the change mentioned in the other signature issue, while we keep thinking about this