Project

Summary

Any transactions that fail based on some conditions that may change in the future are not safe to be executed again later (e.g. transactions that are based on others users actions, could include assets, configs, admin/owner actions, ...etc)

Vulnerability Details

In the current implementation, once the low-level call is failed (occurs frequently due to gas, etc), the whole tx will be reverted and so that nonces[userAddress] = nonces[userAddress] + 1 will remain unchanged.

As a result, the same tx can be replayed by anyone, using the same signature.
https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/meta-transaction/NativeMetaTransaction.sol#L33

Note: Anyone that interacts with executeMetaTransaction (users and admins) could get affected by this issue => A dozen of edge cases are concerned

Quoting sponsor from the cyfrin report at finding 7.3.3:

One World Project: The MetaTransaction’s only intended use is to call the callExternalContract function.The
current implementation is that the EXTERNAL_CALLER signs the transaction in backend and then sends the signed
object to the user and user sends it to the contract by the executeMetaTransaction() function.

While the primary purpose of the executeMetaTransaction is oriented for the EXTERNAL_CALLER this is more of a concern as it would affect and disturb the system, due to replaying the call at an unexepted/undesired period. While it is impossible to cover every edge case as the callExternalContract() function can litteraly do anything (call any contract). But the most likely scenario is replaying a signature oriented to the updateDAOMembership() function (it has the EXTERNAL_CALLER modifier). That could trigger another DOS issue that I've reported which makes the impact more extended.

Please check my report about the DOS attack, assuming you've read the report I'll elaborate about how these 2 issue can be easily coupled. Let's say EXTERNAL_CALLER signs a metaTransaction to initiate updateDAOMembership() that includes updating the amount of each TierConfig. The executeMetaTransaction call fails (due to gas, etc) and the nonce isn't incremented due to the bug. Then later when minted has increased to higher value (minted never decreases in the code!). The attacker replays the signature, and the current amount value is set to the old expired amount value while the current minted value has increased a lot since the old config has been signed, thus triggering the DOS bug mentioned in my report.

Another impact following the same updateDAOMembership() scenario path, an attacker could replay a much older signature that has a very low price setted (likely outdated values). After replaying the sig, the attacker is able to mint tokens at a lower price and siffoning most of funds, by holding the majority of totalSupply within the MembershipERC1155 contract and claiming more profit than other users.

Other serious edge cases are also likely to occur and could be explored, but due to lack of documentation and time. This is basically what we are sure about.

Impact

Any transactions that fail based on some conditions that may change in the future are not safe to be executed again later. Globally speaking this allows an attacker to alter a current DAOs config (while admin action!), causing a DOS or a potential loss of funds through a updateDAOMemebershipoutdated sig impacting all users (within the DAO), and any other unkown impact possible depending on the sig itself (EXTERNAL_CALLER sigs to callExternalContract()!).

Tools Used

Manual Review

Recommendations

This issue and impacts remains as long the nonce isn't incremented for each failed calls, to remediate => Implement OpenZeppelin's MinimalForwarder, I would recommend using the old version (simple and easy implementation), but if needed here you can find the latest version.

While implementing the change above, consider adding one more check to require sufficient gas to be paid, to prevent "Insufficient Gas Griefing Attack" => Add a _gaslimit parameter for each call.