getPreviewModule() Returns Incorrect Data

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x7074de03867b7e8b3843e1dbfe5ce078e4c2f2b0ed6a3867f4da508818c1cda7
Severity: medium

Description:
Description\

getPreviewModule() function in the Helpers.sol returns the 25 modules.However, there is a bug in the external call to safe.getModulesPaginated. In the version of Safe contracts that Palmera is using (version 1.3.0), the getPreviewModules() function returns an incorrect next pointer, resulting in incorrect data being returned. This issue has been fixed in newer versions of Safe contracts, but Palmera still uses version 1.3.0.

/// @dev Method to get Preview Module of the Safe
    /// @param safe address of the Safe
    /// @return address of the Preview Module
    function getPreviewModule(address safe) internal view returns (address) {
        // create Instance of the Safe
        ISafe safeInstance = ISafe(safe);
        // get the modules of the Safe
        (address[] memory modules, address nextModule) =
            safeInstance.getModulesPaginated(address(this), 25);

        if ((modules.length == 0) && (nextModule == Constants.SENTINEL_ADDRESS))
        {
            return Constants.SENTINEL_ADDRESS;
        } else {
            for (uint256 i = 1; i < modules.length;) {
                if (modules[i] == address(this)) {
                    return modules[i - 1];
                }
                unchecked {
                    ++i;
                }
            }
        }
    }

Attack Scenario\

Attachments

1. Proof of Concept (PoC) File

{
  "name": "@gnosis.pm/safe-contracts",
  "version": "1.3.0",<-----------
  "description": "Ethereum multisig contract",
  "homepage": "https://github.com/safe-global/safe-contracts/",
  "license": "GPL-3.0",
  "main": "dist/index.js",
  "typings": "dist/index.d.ts",

https://github.com/safe-global/safe-smart-account/blob/13c0494aca15985023b40c159c94163a4847307d/CHANGELOG.md?plain=1#L202

2. Revised Code File (Optional)

Upgrade to a recent version of Safe.

[AresAudits]

Another Impact found,

The execTransactionOnBehalf function calls checkSignatures, which in turn calls checkNSignatures. In Safe version 1.3.0, checkNSignatures contains a bug. This issue has been fixed in the latest version of Safe contracts.

https://github.com/safe-global/safe-smart-account/blob/13c0494aca15985023b40c159c94163a4847307d/CHANGELOG.md?plain=1#L196

https://docs.safe.global/advanced/smart-account-bug-bounty/past-paid-bounties#:~:text=been%20paid%20out.-,Signature%20verification%20does%20not%20enforce%20a%20maximum%20size%20on%20the%20signature,more%20fees%20than%20it%20would%20have%20with%20optimally%20encoded%20signatures%20bytes.,-The%20workaround%20is

[AresAudits]

@alfredolopez80

[alfredolopez80]

@AresAudits

1. we prefer to still work with version 1.3.0 because is the more used at the moment
2. we guarranty avoid this issue is you check our code, additional we run several onchain test with version 1.4.1 and the ERC-4337 module of safe, and all worked without any issue!!
3. can you indicate in which part of this code the method getPreviewModule _ returns an incorrect next pointer_ as you mention?

https://github.com/safe-global/safe-smart-account/blob/c2d0f0446704fc98ec9d8e568ef238510cffffec/contracts/base/ModuleManager.sol#L114

[AresAudits]

@alfredolopez80 ,
the issue is present in the version 1.3.0 of getModulesPaginated() function. here the getmodulesPaginated() function returns incorrect next module.can u please check the below test case

safe-global/safe-smart-account@743af7f

This will pass the test which is not correct.
await expect(await safe.getModulesPaginated(AddressOne, 1)).to.be.deep.equal([[user3.address], user2.address])
await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], AddressOne])

same issue was found in the brahma contest ,which was then mitigated by updating to latest version

[AresAudits]

@alfredolopez80, let me know if you need any more information

[alfredolopez80]

is a valid issue!!