Open code423n4 opened 2 years ago
This is valid and interesting! Especially because it can be attributed to incompetence instead of maliciousness on behalf of governance whitelisting a faulty module (which can also lead to loss of funds, etc). However, this does emit multiple events with the same publication ID, and thus I believe UIs can filter it, if ever it becomes an issue.
On that front, the mitigation introduces another issue in that incrementing the pubId before creating the comment allows a comment to comment on itself. Paging @miguelmtzinf, wdyt?
Also paging @donosonaumczuk
We acknowledge this and will not be acting on it. I think it's fair to say that if such a vulnerability is found, the double event emission can be a red flag, and governance can act promptly to unwhitelist the specific module. Note that there's already nothing stopping malware or illegal content links from appearing on UIs, who already need to do filtering. Still, this is valid!
Upon first glance, this seems to be valid. An attacker could emit multiple events for a given comment, tricking light clients into responding to a potentially malicious post. However, it requires that reference modules are no longer whitelisted, allowing anyone to register a dodgy module or the governance accidentally registers a faulty module. I think for these reasons, I am more inclined to mark this as medium
as it requires certain assumptions. While I understand C4 typically judges high severity issues as resulting in a loss of funds, Aave Lens is unique in that funds aren't paramount to how the protocol is intended to be used. I am open to further discussion if you disagree with me in any way @Zer0dot ?
I wouldn't put it beyond the realm of possibility to see governance accidentally whitelist a module where this is possible. I do agree it's valid, medium sounds fine to me.
Sweet, I'll downgrade this to medium
considering we are both in agreement.
Lines of code
https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L878-L888
Vulnerability details
Since the Lens platform is a blockchain-based social media platform, it's important that information relevant to users be emitted so that light clients need not continually refer to the blockchain, which can be expensive. From the docs:
As such, it is important that the content of emitted events matches what direct lookups of publication data shows.
Impact
Due to the reentrancy bug outlined below, an attacker is able to emit a comment containing some information that does not match the actual information of a post, allowing him/her to trick light clients into responding to a post that they otherwise would have avoided. The attacker can use this to propagate scams, serve malware, or otherwise poison other user's profiles with unwanted content. Because there is no way to disable publications after the fact, these commenters' profiles now link to this bad content forever.
Proof of Concept
According to the developers in the contest discord, the intention is for the whitelisting of modules to eventually be disabled altogether, or moved to be controlled by a DAO. The main purpose of the whitelist is to make sure that the modules written and used by everyone are built and scoped appropriately, not to limit calls to outside contracts (i.e. the module does what it does in the most efficient manner, using the method requiring the fewest outside contract calls). As such it's reasonable to assume that at some point in the future, an attacker will be able to find or write a ReferenceModule that enables him/her to trigger a function in a contract he/she owns (e.g. transfer an NFT, triggering an ERC721 pre-transfer approval check callback). Below is a version of this where, for simplicity's sake, the malicious code is directly in the module rather than being called by a callback somehow.
As for triggering the actual attack, the attacker first acquires a profile with a lot of followers either by organically growing a following, stealing a profile's NFT, or buying access to one. Next, the attacker publishes interesting content with the malicious ReferenceModule, and finally, the attacker publishes an extremely engaging/viral comment to that publication, which will cause lots of other people to respond to it. The comment will emit an event that contains the original comment information, but the module will be able to overwrite the actual published comment on the blockchain with the attacker's alternate content due to a reentrancy bug where the
pubCount
can be overwritten:https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L878-L888
The following test uses this altered module and shows that the attacker can emit a different comment than is actually stored by/used for subsequent comments:
After applying the above changes, running
npm test test/hub/interactions/publishing-comments.spec.ts
yields:Anyone that has commented on the engaging comment now has unwittingly commented on a malicious URI, potentially encouraging others to visit the URI.
Tools Used
Code inspection Hardhat
Recommended Mitigation Steps
Store the new
pubCount
in a variable before the comment is created and use it during the creation rather than choosing it afterwards.