Malicious user could bypass share lock period and receive maximum curator rewards without ever being a share holder
c4-bot-9 opened this issue · 0 comments
c4-bot-9 commented
Lines of code
Vulnerability details
Summary
Context:
A user can buy shares of a credId using function buyShareCred(). Buying shares does not allow selling for 10 minutes due to the share lock period. This cooldown period was added so that flash loan attacks could not be launched by a malicious user to receive most of the curator rewards through function distribute(). This bug was discovered in the [H-01] finding in the Renascence review here.
Issue:
The issue is that this bug is still not resolved since the lastTradeTimeStamp variable is updated after an external call is made here. This external call is made to provide the caller with any excess payment is made. Note that function safeTransferETH() is used, which forwards all the gas when the excess payment is refunded.
Here is the attack flow:
- Attacker calls buyShareCred() with malicious contract implementation and provides excess payment
- Internal function _handleTrade() refunds the excess to the malicious contract.
- The malicious contract is forwarded all the gas. It can call distribute() on the CuratorRewardsDistributor contract and followingly call sellShareCred() in the fallback.
Impact:
Through this, the attacker was able to bypass the cooldown period to launch the attack (either using own funds or a flash loan), receive most curator rewards from distribute() and sell the shares in the same transaction.
Proof of Concept
- Attacker calls buyShareCred() function with a huge amount of funds (either own or using a flash loan). This calls the internal _handleTrade() function.
File: Cred.sol
178: function buyShareCred(uint256 credId_, uint256 amount_, uint256 maxPrice_) public payable {
179: _handleTrade(credId_, amount_, true, _msgSender(), maxPrice_);
180: }- Here are the important points to observe in function _handleTrade():
- Line 640 updates the attacker's share balance with the amount of tokens provided through the internal function call _updateCuratorShareBalance(). This makes the attacker a valid curator now.
- We enter the if block on Line 642, where the currentSupply is incremented.
- In the if block, the excess refund is made using the safeTransferETH() call to msg.sender first before the lastTradeTimestamp is updated.
File: Cred.sol
591: function _handleTrade(
592: uint256 credId_,
593: uint256 amount_,
594: bool isBuy,
595: address curator_,
596: uint256 priceLimit
597: )
598: internal
599: whenNotPaused
600: {
601: if (amount_ == 0) {
602: revert InvalidAmount();
603: }
604:
605: if (!isExist(credId_)) {
606: revert InvalidCredId();
607: }
608:
609: PhiCred storage cred = creds[credId_];
610:
611: uint256 supply = cred.currentSupply;
612: address creator = cred.creator;
613:
614: (uint256 price, uint256 protocolFee, uint256 creatorFee) =
615: IBondingCurve(cred.bondingCurve).getPriceData(credId_, supply, amount_, isBuy);
616:
617: if (isBuy) {
618: ...
625: }
626: } else {
634: ...
637: }
638: }
639:
640: _updateCuratorShareBalance(credId_, curator_, amount_, isBuy);
641:
642: if (isBuy) {
643: cred.currentSupply += amount_;
644: uint256 excessPayment = msg.value - price - protocolFee - creatorFee;
645: if (excessPayment > 0) {
646:
647: _msgSender().safeTransferETH(excessPayment);
648: }
649: lastTradeTimestamp[credId_][curator_] = block.timestamp;
650: } else {
651: ...
653: }
654:
655: ...
664: }- Since the attacker is a valid curator before the external call is made and the lastTradeTimestamp has not been updated yet, the attacker's malicious contract implementation can receive the refund from its fallback function and call the distribute() function for the credId.
- Since the attacker was added to the curator addresses and the share balance was incremented through _updateCuratorShareBalance(), the attacker receives most of the rewards on Line 112.
- One important thing to note is that the malicious contract implementation would need to deal with receiving the royalty fee that is sent on Line 122. The malicious contract implementation is added at the bottom of this POC to show how the attacker would deal with this.
File: CuratorRewardsDistributor.sol
078: function distribute(uint256 credId) external {
079:
...
086: address[] memory distributeAddresses = credContract.getCuratorAddresses(credId, 0, 0);
089: ...
096:
100: uint256 royaltyfee = (totalBalance * withdrawRoyalty) / RATIO_BASE;
101: uint256 distributeAmount = totalBalance - royaltyfee;
102:
107: uint256 actualDistributeAmount = 0;
108: for (uint256 i = 0; i < distributeAddresses.length; i++) {
109: address user = distributeAddresses[i];
110:
111: uint256 userAmounts = credContract.getShareNumber(credId, user);
112: uint256 userRewards = (distributeAmount * userAmounts) / totalNum;
114: ...
118: }
119:
120: ...
121:
122: _msgSender().safeTransferETH(royaltyfee + distributeAmount - actualDistributeAmount);
123:
124:
125: phiRewardsContract.depositBatch{ value: actualDistributeAmount }(
126: distributeAddresses, amounts, reasons, "deposit from curator rewards distributor"
127: );
128:
129: ...
131: );
132: }- After distribution of all rewards, the fallback function calls the sellShareCred() function to sell the shares, which calls the internal _handleTrade() function.
- We enter the else block on Line 626.
- On Line 629, we check if the share lock period has been adhered to. Since the lastTradeTimestamp has not been updated yet, we pass this check.
- In following lines, the balance of the attacker that was increased is reduced now along with the supply and the attacker receives the tokens back. The attacker malicious contract implementation at the bottom of this pOC also deals with receiving of tokens back through fallback.
File: Cred.sol
591: function _handleTrade(
592: uint256 credId_,
593: uint256 amount_,
594: bool isBuy,
595: address curator_,
596: uint256 priceLimit
597: )
598: internal
599: whenNotPaused
600: {
601: ...
608:
609: PhiCred storage cred = creds[credId_];
610:
611: uint256 supply = cred.currentSupply;
612: address creator = cred.creator;
613:
614: (uint256 price, uint256 protocolFee, uint256 creatorFee) =
615: IBondingCurve(cred.bondingCurve).getPriceData(credId_, supply, amount_, isBuy);
616:
617: if (isBuy) {
618: ...
626: } else {
627: if (priceLimit != 0 && price - protocolFee - creatorFee < priceLimit) revert PriceBelowLimit();
628:
629: if (block.timestamp <= lastTradeTimestamp[credId_][curator_] + SHARE_LOCK_PERIOD) {
630: revert ShareLockPeriodNotPassed(
631: block.timestamp, lastTradeTimestamp[credId_][curator_] + SHARE_LOCK_PERIOD
632: );
633: }
634: (, uint256 nums) = shareBalance[credId_].tryGet(curator_);
635: if (nums < amount_) {
636: revert InsufficientShares();
637: }
638: }
639:
640: _updateCuratorShareBalance(credId_, curator_, amount_, isBuy);
641:
642: if (isBuy) {
643: ...
650: } else {
651: cred.currentSupply -= amount_;
652: curator_.safeTransferETH(price - protocolFee - creatorFee);
653: }
654:
655: ...
664: }Attacker contract implementation
- In the contract below, the attacker deals with the initial excess payment call through the cred contract.
- When distribute() would provide withdraw royalty to us, we won't enter the if block since that is restricted to the credContract only.
- Lastly, when the shares are sold and tokens are received back through fallback, we enter the if block but skip the if block that checks counter == 0 since counter is now 1.
- Through this implementation, the attacker can successfully launch the attack.
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
contract AttackerContract {
bool attackEnabled;
uint256 counter;
bool owner;
address credContract;
address curatorRewardsDistributor;
uint256 credId;
constructor(address cred, address crd) {
owner = msg.sender;
credContract = cred;
curatorRewardsDistributor = crd;
}
// We assume logic to take a flash loan and initiate a buyShareCred() function call is implemented by attacker.
// We also assume logic to retrieve tokens from PhiRewardsContract is implemented.
function enableAttack(bool boolValue) public {
require (msg.sender == owner, "");
attackEnabled = boolValue;
}
function resetCounter() public {
require (msg.sender == owner, "");
counter = 0;
}
function setCredId(uint256 credId_) public {
require (msg.sender == owner, "");
credId = credId_;
}
fallback() external payable {
if (attackEnabled && msg.sender == credContract) {
if (counter == 0) {
counter++;
// External function call to distribute(credId)
// External function call to sellShareCred()
}
}
}
Tools Used
Manual Review
Recommended Mitigation Steps
When refunding msg.value, use forceSafeTransferETH() from SafeTransferLib. Additionally, add a reentrant modifiers to the buy and sell share cred functions.
Another protective measure to add would be to update the lastTradeTimestamp variable before the refund is made.
Assessed type
Reentrancy