code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/follow/FeeFollowModule.sol#L82 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/MultirecipientFeeCollectModule.sol#L185 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/base/BaseFeeCollectModule.sol#L190 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/base/BaseFeeCollectModule.sol#L219 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/base/BaseFeeCollectModule.sol#L274

Vulnerability details

Impact

User can follow a ProfileId via LensHub.follow. If the Profile sets followModule, then FeeFollowModule.processFollow will be called to collect fees from the caller. If the currency is GUSD, the treasuryAmount may be 0 due to rounding down, then the protocol will not receive the fee and suffer fund loss.

Proof of Concept

Let's look at the code of FeeFollowModule.processFollow.

File: contracts\modules\follow\FeeFollowModule.sol
67:     function processFollow(
68:         uint256 /* followerProfileId */,
69:         uint256 followTokenId,
70:         address transactionExecutor,
71:         uint256 targetProfileId,
72:         bytes calldata data
73:     ) external override onlyHub returns (bytes memory) {
74:         // We charge only when performing a fresh follow.
75:         if (followTokenId == 0) {
76:             uint256 amount = _feeConfig[targetProfileId].amount;
77:             address currency = _feeConfig[targetProfileId].currency;
78:             _validateDataIsExpected(data, currency, amount);
79: 
80:             (address treasury, uint16 treasuryFee) = _treasuryData();
81:             address recipient = _feeConfig[targetProfileId].recipient;
82:->           uint256 treasuryAmount = (amount * treasuryFee) / BPS_MAX;
83:             uint256 adjustedAmount = amount - treasuryAmount;
84: 
85:             IERC20(currency).safeTransferFrom(transactionExecutor, recipient, adjustedAmount);
86:             if (treasuryAmount > 0) {
87:                 IERC20(currency).safeTransferFrom(transactionExecutor, treasury, treasuryAmount);
88:             }
89:         } else {
......
97:     }

The assumptions are as follows:

_feeConfig[targetProfileId].currency = GUSD(decimals=2)
_feeConfig[targetProfileId].amount=900($9)
treasuryFee=10 (0.1%)
BPS_MAX = 10000

According to the formula of L82, treasuryAmount = (900 * 10) / 10000 = 0. In this way, the treasury will never receive such a fee. Imagine that if targetProfileId is owned by a big name, who has so many fans. One fan will transfer $0.09 to treasury, and 100 million fans will transfer $900M to treasury. In this case, the protocol's loss is huge.

MultirecipientFeeCollectModule/BaseFeeCollectModule also has this issue. It has been marked in Links to affected code, so skip it here.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not add tokens with decimals less than 6 to the whitelist.

Assessed type

Decimal

141345 commented 1 year ago

low decimal token rounding issue

QA might be more appropriate.

donosonaumczuk commented 1 year ago

Module contracts are out of scope
The recommendation "Do not add tokens with decimals less than 6 to the whitelist" is part of the assumptions listed in the README. Governance will whitelist tokens that make sense for the protocol.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)