Title 1: In _buyCurvesToken function, the left amount of ETH sent by the buyer is not refunded to him.
High
If the amount of Ether to be sent is greater than the amount actually needed in _buyCurvesToken function, the left amount will not be refunded to the buyer. Therefore, the buyer will lose the left amount.
In _buyCurvesToken function, they calculates the price and fees that to be needed for buying curves token of certain amount and compare if received ETH is greater than sum of the price and fees.
uint256 price = getPrice(supply, amount);
(, , , , uint256 totalFee) = getFees(price);
if (msg.value < price + totalFee) revert InsufficientPayment();But they don't refund the left amount of ETH to the buyer. Total supply of the curves token may change before the transaction is executed due to various actions like selling curves token by the other account and it affects the price and fees. Therefore, received ETH amount may become greater than needed amount indeedly and buyer will lost left ETH because left amount is not refunded.
VS Code
Refund left amount of ETH to the account when received ETH is greater than sum of price and totalFee.
Title 2: When an account sell its curves token, the protocol fee is not transfered to protocolFeeDestination.
High
When an account sell its curves token, the protocol is calculated but not transfered to anywhere.
When an account sell its curves token, the internal function _transferFees is executed to distribute fees to right destinations.
function _transferFees(
address curvesTokenSubject,
bool isBuy,
uint256 price,
uint256 amount,
uint256 supply
) internal {This function has isBuy param and it is passed as false in the case of selling. Otherwise, it is passed as true in the case of buying
address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
(bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
if (!success1) revert CannotSendFunds();If isBuy is true, firstDestination is set as priceFeeDestination and the protocol fee is transfered to it. But if isBuy is false, firstDestination is set as seller and calculated protocol fee isn't transfered to anywhere.
Therefore, the protocol will lose their fee amount about sellCurvesToken action.
VS Code
Add logic that send protocol fee to right destination when isBuy is false in _transferFees function.
function _transferFees(
address curvesTokenSubject,
bool isBuy,
uint256 price,
uint256 amount,
uint256 supply
) internal {
(uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee, ) = getFees(price);
{
bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
{
address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
(bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
if (!success1) revert CannotSendFunds();
+ if (!isBuy) {
+ (bool success4, ) = protocolFeeDestination.call{value: buyValue}("");
+ if (!success4) revert CannotSendFunds();
+ }
}
{
(bool success2, ) = curvesTokenSubject.call{value: subjectFee}("");
if (!success2) revert CannotSendFunds();
}
{
(bool success3, ) = referralDefined
? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("")
: (true, bytes(""));
if (!success3) revert CannotSendFunds();
}
if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
}
}
emit Trade(
msg.sender,
curvesTokenSubject,
isBuy,
amount,
price,
protocolFee,
subjectFee,
isBuy ? supply + amount : supply - amount
);
}Title 3: As the length of an account's ownedCurvesTokenSubjects continues to increase, a DoS may occur when that account purchases new curves tokens.
High
As the number of curves tokens owned by an account increases, buying new curve token of the account may fall into DoS.
Whenever an account buy a new curve token, the token is pushed to ownedCurvesTokenSubjects of the account.
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
for (uint256 i = 0; i < subjects.length; i++) {
if (subjects[i] == curvesTokenSubject) {
return;
}
}
subjects.push(curvesTokenSubject);
}But there is no any logic that decrease the length ownedCurvesTokenSubjects[_owned] in entire protocol so that it would be increasing whenever the account buy new curve token. It may exceed gas limit of transaction due to for loop with too long array length and fall into denial of service.
VS Code
In _addOwnedCurvesTokenSubject function, change the logic that check if passed curvesTokenSubject is already owned token by introducing new state ownedCurvesTokenSubjectStatus.
+ mapping(address => mapping(address => bool)) public ownedCurvesTokenSubjectStatus;
...
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
- for (uint256 i = 0; i < subjects.length; i++) {
- if (subjects[i] == curvesTokenSubject) {
- return;
- }
- }
+ if (ownedCurvesTokenSubjectStatus[owner_][curvesTokenSubject]) {
+ return;
+ } else {
+ ownedCurvesTokenSubjectStatus[owner_][curvesTokenSubject] = true;
+ }
subjects.push(curvesTokenSubject);
}High
Since setCurves function of FeeSplitter contract isn't be protected by a trusted role, anyone can change curves contract address.
setCurves function of FeeSplitter contract is public function but it doesn't have any modifier that protects it with trusted role.
function setCurves(Curves curves_) public {
curves = curves_;
}Therefore, anyone can change curves address of the contract and it may occur error while performing transaction.
In worst case, an attacker can manipulate balance and supply of particular curve token by deploying own mocked Curves contract and setting its address as curves of FeeSplitter.
VS Code
Add modifier that protect with a trusted role to setCurves function.
Medium
In setWhitelist function of Curves contract, merkleRoot can be zero and anyone can be whitelisted since there is no validation.
In setWhitelist function of Curves contract, there isn't any validation for merkleRoot.
function setWhitelist(bytes32 merkleRoot) external {
uint256 supply = curvesTokenSupply[msg.sender];
if (supply > 1) revert CurveAlreadyExists();
if (presalesMeta[msg.sender].merkleRoot != merkleRoot) {
presalesMeta[msg.sender].merkleRoot = merkleRoot;
emit WhitelistUpdated(msg.sender, merkleRoot);
}
}Therefore, merkleRoot can be zero and it allows everyone can be whitelisted.
VS Code
Add validation for merkleRoot as follow.
function setWhitelist(bytes32 merkleRoot) external {
uint256 supply = curvesTokenSupply[msg.sender];
if (supply > 1) revert CurveAlreadyExists();
+ require(merkleRoot != bytes32(0), "MerkleRoot can't be zero")
if (presalesMeta[msg.sender].merkleRoot != merkleRoot) {
presalesMeta[msg.sender].merkleRoot = merkleRoot;
emit WhitelistUpdated(msg.sender, merkleRoot);
}
}Title 6: Holders can lose their holder fee due to updating of userFeeOffset of onBalanceChange function
Medium
When holders buy/sell the curves tokens, they can't claim the past cumulativeFeePerToken since userFeeOffset is updated in onBalanceChange function.
When holders buy/sell the curves tokens, _transferFees function is called to distribute fees.
function _transferFees(
address curvesTokenSubject,
bool isBuy,
uint256 price,
uint256 amount,
uint256 supply
) internal {
(uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee, ) = getFees(price);
{
bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
{
address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
(bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
if (!success1) revert CannotSendFunds();
}
{
(bool success2, ) = curvesTokenSubject.call{value: subjectFee}("");
if (!success2) revert CannotSendFunds();
}
{
(bool success3, ) = referralDefined
? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("")
: (true, bytes(""));
if (!success3) revert CannotSendFunds();
}
if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
}
}
emit Trade(
msg.sender,
curvesTokenSubject,
isBuy,
amount,
price,
protocolFee,
subjectFee,
isBuy ? supply + amount : supply - amount
);
}If holdersFeePercent is greater than 0, onBalanceChange function of FeeSplitter contract is called.
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);
}In onBalanceChange function, data.userFeeOffset[account] is updated as current value of data.cumulativeFeePerToken. Therefore, the holder lose the past cumulativeFeePerToken unless he claims it in advance before buy/sell the curves token.
VS Code
The code line that updates userFeeOffset[account] in onBalanceChange function should be removed.
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);
}Title 7: When a user tries to deploy new curves token with default name and symbol, the malicious user can make it would be reverted.
Medium
When subjects try to deploy their curves token by using the default name and symbol, a malicious user can make it would be reverted.
When a subject tries to deploy a new own curves token with the name and symbol, _deployERC20 function of Curves.sol contract is called and checks if the passed symbol alreday exist.
function _deployERC20(
address curvesTokenSubject,
string memory name,
string memory symbol
) internal returns (address) {
// If the token's symbol is CURVES, append a counter value
if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) {
_curvesTokenCounter += 1;
name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter)));
symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter)));
}
@> if (symbolToSubject[symbol] != address(0)) revert InvalidERC20Metadata();
address tokenContract = CurvesERC20Factory(curvesERC20Factory).deploy(name, symbol, address(this));
externalCurvesTokens[curvesTokenSubject].token = tokenContract;
externalCurvesTokens[curvesTokenSubject].name = name;
externalCurvesTokens[curvesTokenSubject].symbol = symbol;
externalCurvesToSubject[tokenContract] = curvesTokenSubject;
symbolToSubject[symbol] = curvesTokenSubject;
emit TokenDeployed(curvesTokenSubject, tokenContract, name, symbol);
return address(tokenContract);
}Also, if a subject tries to deploy new curves token without setting its name and symbol in advance by calling mint function, the default name and symbol would be used. And this can be a problem.
Let me explain with an example. Malicious users can perform the following steps to occur problem:
-
First, a malicious user get the vlaue of state
_curvesTokenCounter. (Assume that this value is 3). Getting this private value is not impossible. -
Next, the malicious user calls
buyCurvesTokenWithNamefunction with params ofnameasstring(abi.encodePacked(DEFAULT_NAME, " ", Strings.toString(4)))andsymbolasstring(abi.encodePacked(DEFAULT_SYMBOL, Strings.toString(4))).function buyCurvesTokenWithName( address curvesTokenSubject, uint256 amount, string memory name, string memory symbol ) public payable { uint256 supply = curvesTokenSupply[curvesTokenSubject]; if (supply != 0) revert CurveAlreadyExists(); _buyCurvesToken(curvesTokenSubject, amount); _mint(curvesTokenSubject, name, symbol); }
-
As the result,
_mintfunction is called and then_deployERC20function is called with thenameandsymbol. The passed symbol is different fromDEFAULT_SYMBOLand it allows the malicious user own the curves token with symbol without increasing the_curvesTokenCounter.if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) { _curvesTokenCounter += 1; name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter))); symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter))); }
In this situation, when another user tries to deploy new curves token with default name and symbol, it will always be reverted because symbolToSubject[string(abi.encodePacked(DEFAULT_SYMBOL, Strings.toString(4)))] is already set as true in above steps by following codeline.
if (symbolToSubject[symbol] != address(0)) revert InvalidERC20Metadata();In withdraw function, the same problem may occur.
VS Code
Move the codeline that increasing _curvesTokenCounter as follow:
function _deployERC20(
address curvesTokenSubject,
string memory name,
string memory symbol
) internal returns (address) {
// If the token's symbol is CURVES, append a counter value
+ _curvesTokenCounter += 1;
if (keccak256(bytes(symbol)) == keccak256(bytes(DEFAULT_SYMBOL))) {
- _curvesTokenCounter += 1;
name = string(abi.encodePacked(name, " ", Strings.toString(_curvesTokenCounter)));
symbol = string(abi.encodePacked(symbol, Strings.toString(_curvesTokenCounter)));
}
if (symbolToSubject[symbol] != address(0)) revert InvalidERC20Metadata();Title 8: Because the transferCurvesToken function doesn't check if amount is greater than 0, a malicious user can permanently block other users from buying new curves tokens and depositing.
Medium
A malicious user can increase another user's number of owned curves token massively by using transferCurvesToken function and this may occur DoS when the victim tries to buy new curves token or call deposit function.
Users can transfer their tokens to the others by using transferCurvesToken function.
But, as you can see, this function doesn't check if amount is greater than 0.
function transferCurvesToken(address curvesTokenSubject, address to, uint256 amount) external {
if (to == address(this)) revert ContractCannotReceiveTransfer();
_transfer(curvesTokenSubject, msg.sender, to, amount);
}
// ...
function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();
// If transferring from oneself, skip adding to the list
if (from != to) {
_addOwnedCurvesTokenSubject(to, curvesTokenSubject);
}
curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;
emit Transfer(curvesTokenSubject, from, to, amount);
}
// Internal function to add a curvesTokenSubject to the list if not already present
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
for (uint256 i = 0; i < subjects.length; i++) {
if (subjects[i] == curvesTokenSubject) {
return;
}
}
subjects.push(curvesTokenSubject);
}Therefore, a malicious user can increase another user's ownedCurvesTokenSubjects by calling this transferCurvesToken function with params of curvesTokenSubject as an arbitrary address, to as the victim's address and amount as 0.
If the malicious user repeats above action massively with different curvesTokenSubject param each time, it is possible that reach to the block gas limit due to following for loop.
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
@> for (uint256 i = 0; i < subjects.length; i++) {
if (subjects[i] == curvesTokenSubject) {
return;
}
}
subjects.push(curvesTokenSubject);
}Since _buyCurvesToken and _transfer use this _addOwnedCurvesTokenSubject function, the victim cannot buy new curves token and deposit.
function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
uint256 supply = curvesTokenSupply[curvesTokenSubject];
if (!(supply > 0 || curvesTokenSubject == msg.sender)) revert UnauthorizedCurvesTokenSubject();
uint256 price = getPrice(supply, amount);
(, , , , uint256 totalFee) = getFees(price);
if (msg.value < price + totalFee) revert InsufficientPayment();
curvesTokenBalance[curvesTokenSubject][msg.sender] += amount;
curvesTokenSupply[curvesTokenSubject] = supply + amount;
_transferFees(curvesTokenSubject, true, price, amount, supply);
// If is the first token bought, add to the list of owned tokens
if (curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0) {
_addOwnedCurvesTokenSubject(msg.sender, curvesTokenSubject);
}
}
// ..
function deposit(address curvesTokenSubject, uint256 amount) public {
if (amount % 1 ether != 0) revert NonIntegerDepositAmount();
address externalToken = externalCurvesTokens[curvesTokenSubject].token;
uint256 tokenAmount = amount / 1 ether;
if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();
CurvesERC20(externalToken).burn(msg.sender, amount);
_transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);
}VS Code
You should revert if amount is not greater than 0, in _transfer function.
function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
+ if (!(amount > 0)) revert;
if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();
// If transferring from oneself, skip adding to the list
if (from != to) {
_addOwnedCurvesTokenSubject(to, curvesTokenSubject);
}
curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;
emit Transfer(curvesTokenSubject, from, to, amount);
}Title 9: Because the transferCurvesToken function doesn't check if amount is greater than 0, a malicious user can permanently block other users from buying new curves tokens and depositing.
Medium
A malicious user can increase another user's number of owned curves token massively by using transferCurvesToken function and this may occur DoS when the victim tries to buy new curves token or call deposit function.
Users can transfer their tokens to the others by using transferCurvesToken function.
But, as you can see, this function doesn't check if amount is greater than 0.
function transferCurvesToken(address curvesTokenSubject, address to, uint256 amount) external {
if (to == address(this)) revert ContractCannotReceiveTransfer();
_transfer(curvesTokenSubject, msg.sender, to, amount);
}
// ...
function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();
// If transferring from oneself, skip adding to the list
if (from != to) {
_addOwnedCurvesTokenSubject(to, curvesTokenSubject);
}
curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;
emit Transfer(curvesTokenSubject, from, to, amount);
}
// Internal function to add a curvesTokenSubject to the list if not already present
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
for (uint256 i = 0; i < subjects.length; i++) {
if (subjects[i] == curvesTokenSubject) {
return;
}
}
subjects.push(curvesTokenSubject);
}Therefore, a malicious user can increase another user's ownedCurvesTokenSubjects by calling this transferCurvesToken function with params of curvesTokenSubject as an arbitrary address, to as the victim's address and amount as 0.
If the malicious user repeats above action massively with different curvesTokenSubject param each time, it is possible that reach to the block gas limit due to following for loop.
function _addOwnedCurvesTokenSubject(address owner_, address curvesTokenSubject) internal {
address[] storage subjects = ownedCurvesTokenSubjects[owner_];
@> for (uint256 i = 0; i < subjects.length; i++) {
if (subjects[i] == curvesTokenSubject) {
return;
}
}
subjects.push(curvesTokenSubject);
}Since _buyCurvesToken and _transfer use this _addOwnedCurvesTokenSubject function, the victim cannot buy new curves token and deposit.
function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
uint256 supply = curvesTokenSupply[curvesTokenSubject];
if (!(supply > 0 || curvesTokenSubject == msg.sender)) revert UnauthorizedCurvesTokenSubject();
uint256 price = getPrice(supply, amount);
(, , , , uint256 totalFee) = getFees(price);
if (msg.value < price + totalFee) revert InsufficientPayment();
curvesTokenBalance[curvesTokenSubject][msg.sender] += amount;
curvesTokenSupply[curvesTokenSubject] = supply + amount;
_transferFees(curvesTokenSubject, true, price, amount, supply);
// If is the first token bought, add to the list of owned tokens
if (curvesTokenBalance[curvesTokenSubject][msg.sender] - amount == 0) {
_addOwnedCurvesTokenSubject(msg.sender, curvesTokenSubject);
}
}
// ..
function deposit(address curvesTokenSubject, uint256 amount) public {
if (amount % 1 ether != 0) revert NonIntegerDepositAmount();
address externalToken = externalCurvesTokens[curvesTokenSubject].token;
uint256 tokenAmount = amount / 1 ether;
if (externalToken == address(0)) revert TokenAbsentForCurvesTokenSubject();
if (amount > CurvesERC20(externalToken).balanceOf(msg.sender)) revert InsufficientBalance();
if (tokenAmount > curvesTokenBalance[curvesTokenSubject][address(this)]) revert InsufficientBalance();
CurvesERC20(externalToken).burn(msg.sender, amount);
_transfer(curvesTokenSubject, address(this), msg.sender, tokenAmount);
}VS Code
You should revert if amount is not greater than 0, in _transfer function.
function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
+ if (!(amount > 0)) revert;
if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();
// If transferring from oneself, skip adding to the list
if (from != to) {
_addOwnedCurvesTokenSubject(to, curvesTokenSubject);
}
curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;
emit Transfer(curvesTokenSubject, from, to, amount);
}