c4-bot-2 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L491

Vulnerability details

Impact

In the deposit() function currently, only integer amounts are allowed to be deposited. Although this is expected behaviour, the issue with this is that users will not always have whole integer amount of ERC20 tokens with them. This is because these ERC20 tokens will be sitting in Uniswap pools, bridge lockers or even yield bearing vaults that themselves allow to earn more of the same ERC20 tokens staked (see sponsor comment here under FeeSplitter). Users can also receive non-integer amount of ERC20 tokens from their friends as gifts or other means of distributions.

The tokens earned from these strategies will not always be integer amounts. Due to this, when a user calls deposit(), they will only be able to do so upto the closest integer number while the remainder amount is not able to be reintegrated.

This issue expands as more ERC20 tokens are exported from the Curves contract since the tokens are transferrable and will be open for dilution.

The last resort for users in such a case would be to buy more shares on Curves and export them but we cannot assume that every user has a lot of purchasing power.

Due to this, the issue has been marked Medium-severity.

Proof of Concept

Coded POC

Here is how to run this POC:

Integrate foundry into the project through this
Add the POC in the test folder.
Run forge test --match-test testReintegrationIssue -vvv

File: testERC20ReIntegrationIssue.t.sol
001: // SPDX-License-Identifier: MIT
002: pragma solidity ^0.8.7;
003: 
004: import {Curves} from "../contracts/Curves.sol";
005: import {CurvesERC20Factory} from "../contracts/CurvesERC20Factory.sol";
006: import {FeeSplitter} from "../contracts/FeeSplitter.sol";
007: import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
008: import {Test} from "forge-std/Test.sol";
009: import {console} from "forge-std/console.sol";
010: 
011: contract testCurves is Test {
012: 
013:     // Contract instances
014:     Curves curves;
015:     CurvesERC20Factory factory;
016:     FeeSplitter fs;
017: 
018:     // Fee destinations
019:     address public protocolFeeDestination = makeAddr("protocolFeeDestination");
020:     address public referralFeeDestination = makeAddr("referralFeeDestination");
021:  
022:     // Token subject
023:     address public alice = makeAddr("alice");
024: 
025:     // Sample user
026:     address public bob = makeAddr("bob");
027: 
028:     // Yield distributor
029:     address public yieldDistributor = makeAddr("yielder");
030: 
031: 
032:     function setUp() public {
033:         factory = new CurvesERC20Factory();
034:         fs = new FeeSplitter();
035:         curves = new Curves(address(factory), address(fs));
036:     }
037: 
038:     function testSetupFees() public {
039:         // 20% - Protocol fees
040:         // 20% - Subject fees
041:         // 10% - Referral fees
042:         // 50% - Holder fees
043:         curves.setMaxFeePercent(1e18);
044:         curves.setProtocolFeePercent(2e17, protocolFeeDestination); 
045:         curves.setExternalFeePercent(2e17, 1e17, 5e17);
046:         vm.prank(alice);
047:         curves.setReferralFeeDestination(alice, referralFeeDestination);
048:     }
049: 
050:     function testBuyFirstCurveToken() public {
051:         vm.prank(alice);
052:         curves.buyCurvesToken(alice, 1);
053:     }
054: 
055:     function testUsersBuyCurvesToken() public {
056:         uint256 buyPriceAfterFee1 = curves.getBuyPriceAfterFee(alice, 99);
057:         vm.deal(bob, buyPriceAfterFee1);
058:         vm.prank(bob);
059:         curves.buyCurvesToken{value: buyPriceAfterFee1}(alice, 99);
060: 
061:         uint256 buyPriceAfterFee2 = curves.getBuyPriceAfterFee(alice, 99);
062:         vm.deal(yieldDistributor, buyPriceAfterFee2);
063:         vm.prank(yieldDistributor);
064:         curves.buyCurvesToken{value: buyPriceAfterFee2}(alice, 99);
065:         //console.log("Yield balance:", curves.curvesTokenBalance(alice, yieldDistributor) * 1e18);
066:     }
067: 
068:     function testExportTokens() public {
069:         curves.withdraw(alice, 0);
070: 
071:         vm.prank(yieldDistributor);
072:         curves.withdraw(alice, 99);
073: 
074:         vm.prank(bob);
075:         curves.withdraw(alice, 99);
076:     }
077: 
078:     function testReintegrationIssue() public {
079:         testSetupFees();
080: 
081:         fs.setCurves(Curves(address(curves)));
082:         testBuyFirstCurveToken();
083: 
084:         testUsersBuyCurvesToken();
085: 
086:         testExportTokens();
087: 
088:         // Bob receives yield from yieldDistributor, which is external to Curves
089:         (string memory name, string memory symbol, address token) = curves.externalCurvesTokens(alice);
090:         vm.prank(yieldDistributor);
091:         ERC20(token).transfer(bob, 10111111111111111); // ERC20 tokens to bob from yield
092: 
093:         // Bob is not able to deposit whole amount
094:         uint256 bobBalance = ERC20(token).balanceOf(bob);
095:         vm.prank(bob);
096:         vm.expectRevert();
097:         curves.deposit(alice, bobBalance);
098: 
099:         // Bob deposits integer amount
100:         vm.prank(bob);
101:         curves.deposit(alice, 99 * 1e18);
102: 
103:         console.log("Bob's non-reintegrable tokens:", ERC20(token).balanceOf(bob));
104:     }
105: }

Issue Confirmation

Imgur

Tools Used

Manual Review

Recommended Mitigation Steps

Since we cannot go break the conversion from the 18 decimals format in the deposit() function, a good straightforward solution to this would be to introduce a vault for the users by the Curves team. This vault can allow users to deposit their ERC20 curves tokens. Once the number of tokens for a token subject in the vault reaches a whole integer number, an administrator can call sellExternalCurveTokens() on the Curves contract which will give the vault ETH, that can then be distributed among the users who deposited their ERC20 tokens in it. The ETH would obviously need to be distributed based on the ratio or weight the users provide.

There can be better solutions than this but in this simple solution above I've tried to ensure no new component other than a vault is introduced to aggregate the tokens to a whole integer number.

Assessed type

Other

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #210

c4-judge commented 9 months ago

alcueca changed the severity to QA (Quality Assurance)