diff --git a/README.md b/README.md index 5458141..794fb7c 100644 --- a/README.md +++ b/README.md @@ -371,7 +371,132 @@ Cost_2: 0.711616420426520863 ETH ## Mitigation It would be advisable to introduce a time delay between any consecutive calls to buy or sell votes by the same address. This would ensure price manipulation can't happen in the same block, thus mitigating the attack. -# Issue H-2: Market funds cannot be withdrawn because of incorrect calculation of `fundsPaid` + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/trust-ethos/ethos/pull/2214 + + +# Issue H-2: Users could overpay fees when buying votes + +Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/314 + +## Found by +0x486776, 0xPhantom2, 0xaxaxa, 0xc0ffEE, 0xlucky, Abhan1041, BengalCatBalu, DenTonylifer, John44, Ryonen, X12, aycozyOx, bughuntoor, chaos304, dobrevaleri, hals, irresponsible, newspacexyz, novaman33, pashap9990, smbv-1923, tmotfl, underdog, vatsal, wellbyt3, ydlee, zxriptor +### Summary + +The `previewFees` function in `_calculateBuy` is applied to the total `funds` being transferred by the user. This leads to users paying for more funds that they are actually transacting. + +### Root Cause + +In [`ReputationMarket:960`](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L960), users will specify an amount of `funds` that they are willing to pay in exchange for votes. However, the specified `funds` might not be fully used in order to buy the votes, given the following logic in `_calculateBuy`: + +```solidity +// ReputationMarket.sol + +function _calculateBuy( + Market memory market, + bool isPositive, + uint256 funds + ) + private + view + returns ( + uint256 votesBought, + uint256 fundsPaid, + uint256 newVotePrice, + uint256 protocolFee, + uint256 donation, + uint256 minVotePrice, + uint256 maxVotePrice + ) + { + uint256 fundsAvailable; + (fundsAvailable, protocolFee, donation) = previewFees(funds, true); + uint256 votePrice = _calcVotePrice(market, isPositive); + + uint256 minPrice = votePrice; + uint256 maxPrice; + + if (fundsAvailable < votePrice) { + revert InsufficientFunds(); + } + + while (fundsAvailable >= votePrice) { + fundsAvailable -= votePrice; + fundsPaid += votePrice; + votesBought++; + + market.votes[isPositive ? TRUST : DISTRUST] += 1; + votePrice = _calcVotePrice(market, isPositive); + } + fundsPaid += protocolFee + donation; + + maxPrice = votePrice; + + return (votesBought, fundsPaid, votePrice, protocolFee, donation, minPrice, maxPrice); + } + +``` + +As shown in the snippet, the amount of votes bought is determined by a loop that will run while the `fundsAvailable` are greater than the `votePrice`. As the price of votes increases due to the buy pressure, the loop will be finished, having `fundsAvailable` (which consists of the user's submitted `funds` with the fees substracted) be nearly always greater than the actual `fundsPaid` (which consists of the price paid for each vote + fees + donation fee). + +The problem with this approach is that fees are being applied to an amount that, as demonstrated, is not necessarily the total amount used to actually buy the votes. + + + + +### Internal pre-conditions + +None + +### External pre-conditions + +None + +### Attack Path + +Let's say a user 1 wants to buy 2 `TRUST` votes for a recently created market, where `basePrice` is 0,01 ETH and theres 1 `TRUST` and 1 `DISTRUST` vote already in the market. + +1. The price of one vote is given by `market.votes[isPositive ? TRUST : DISTRUST] * market.basePrice) / totalVotes`, so the price is 0,005 ETH (1 * 0,01 / 2) for the first vote, and ≈ 0,0066 (2 * 0,01 / 3) for the second vote, which adds up to a total of 0,0116 ETH. A 10 % fee is applied (so fee is 0,00116), so the total that user should deposit is 0,01276. + +2. At the same time, and prior to user 1 buying the votes, user 2 submits a buy transaction for one `TRUST` vote. This leaves the state of the vote prices to a different price than the expected by user 1. Still, user 1 submits the transaction with a value of 0,01276. +3. Because user 2 has triggered the buy operation prior to user 1, the initial vote price for user 1 is 0,0066 ETH (2 * 0,01 / 3). As user 1 submitted 0,01276 as `funds`, and without the 10% in fees, the `fundsAvailable` for user 1 are 0,011484. Note that 0,001276 are paid in fees. +4. While in the loop: +- First iteration: `votePrice` starts at 0,0066, and `fundsAvailable` are 0,011484, so one vote is purchased. The `fundsPaid` increases to 0,0066. +- Second iteration: `votePrice` now is at 0,0075 (3 * 0,01 / 4). `fundsAvailable` are 0,004884, so it is not enough to buy a second vote +5. The result is that user 1 has only been able to purchase a single vote. The actual transacted value has only been 0,0066 (the price of one vote), for which a 10% fee would have implied 0,00066 ETH (≈ 2,442 USD). However, as shown in step 1, user 1 has paid 0,00116 ETH (≈ 4,292 USD, **nearly the double!**). + +On the long run, situations like this will arise, leading to a perpetual loss of funds for protocol users due to the increase + +### Impact + +Medium. As shown in the "Attack Path" section, fees will be overcharged for users that buy votes. On the long term, it is easy that the total value loss exceeds 10 USD or 10% of user value, (considering that new markets might also be configured, with base prices of 0,1 or even 1 ETH). + + +### PoC + +_No response_ + +### Mitigation + +When triggering `buyVotes`, allow users to specify the amount of votes they want, instead of the amount of ETH to pay (similar to the logic used when selling). Then, apply the corresponding fees considering the actual amount that user will pay. + + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/trust-ethos/ethos/pull/2214 + + +# Issue H-3: Market funds cannot be withdrawn because of incorrect calculation of `fundsPaid` Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/660 @@ -477,12 +602,12 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/trust-ethos/ethos/pull/2216 -# Issue H-3: A user can pay less in fees by vouching initially with a smaller amount and then using the `EthosVouch::increaseVouch` function to add the remaining vouch value +# Issue H-4: A user can pay less in fees by vouching initially with a smaller amount and then using the `EthosVouch::increaseVouch` function to add the remaining vouch value Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/714 ## Found by -0xKann, 0xPhantom2, Bbash, BengalCatBalu, John44, POB, Pheonix, Ryonen, SovaSlava, X12, debugging3, mahdikarimi, shui, t0x1c, tachida2k, tjonair, zxriptor +0xKann, 0xPhantom2, Bbash, BengalCatBalu, John44, POB, Pheonix, Ryonen, SovaSlava, X12, debugging3, mahdikarimi, moray5554, shui, t0x1c, tachida2k, tjonair, zxriptor ### Summary A vulnerability in the `EthosVouch` fee mechanism allows users to reduce fees when vouching for a subject. By splitting their vouching process into multiple smaller transactions, users can partially reclaim `vouchersPoolFee`, resulting in significantly lower total fees compared to a single large transaction. This exploit undermines the intended fee structure and results in financial losses for other previous vouchers. @@ -537,102 +662,69 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/trust-ethos/ethos/pull/2242 -# Issue M-1: Users could overpay fees when buying votes +# Issue M-1: authorProfileId can avoid being slashed -Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/314 +Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/1 ## Found by -0xaxaxa, DenTonylifer, pashap9990, underdog +0xAnmol, 0xPhantom2, 0xbakeng, 0xc0ffEE, BengalCatBalu, Contest-Squad, LeFy, X12, farismaulana, newspacexyz, rmdanxyz, volodya ### Summary -The `previewFees` function in `_calculateBuy` is applied to the total `funds` being transferred by the user. This leads to users paying for more funds that they are actually transacting. +there is not lock lock on lock on staking (and withdrawals) for the accused authorProfileId ### Root Cause -In [`ReputationMarket:960`](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L960), users will specify an amount of `funds` that they are willing to pay in exchange for votes. However, the specified `funds` might not be fully used in order to buy the votes, given the following logic in `_calculateBuy`: - +According to [docs](https://whitepaper.ethos.network/ethos-mechanisms/slash) their should be lock +> Any Ethos participant may act as a "whistleblower" to accuse another participant of inaccurate claims or unethical behavior. This accusation triggers a 24h lock on staking (and withdrawals) for the accused. +Currently anyone can unvouch at any time ```solidity -// ReputationMarket.sol - -function _calculateBuy( - Market memory market, - bool isPositive, - uint256 funds - ) - private - view - returns ( - uint256 votesBought, - uint256 fundsPaid, - uint256 newVotePrice, - uint256 protocolFee, - uint256 donation, - uint256 minVotePrice, - uint256 maxVotePrice - ) - { - uint256 fundsAvailable; - (fundsAvailable, protocolFee, donation) = previewFees(funds, true); - uint256 votePrice = _calcVotePrice(market, isPositive); - - uint256 minPrice = votePrice; - uint256 maxPrice; - - if (fundsAvailable < votePrice) { - revert InsufficientFunds(); + function unvouch(uint256 vouchId) public whenNotPaused nonReentrant { + Vouch storage v = vouches[vouchId]; + _vouchShouldExist(vouchId); + _vouchShouldBePossibleUnvouch(vouchId); + // because it's $$$, you can only withdraw/unvouch to the same address you used to vouch + // however, we don't care about the status of the address's profile; funds are always attached + // to an address, not a profile + if (vouches[vouchId].authorAddress != msg.sender) { + revert AddressNotVouchAuthor(vouchId, msg.sender, vouches[vouchId].authorAddress); } - while (fundsAvailable >= votePrice) { - fundsAvailable -= votePrice; - fundsPaid += votePrice; - votesBought++; - - market.votes[isPositive ? TRUST : DISTRUST] += 1; - votePrice = _calcVotePrice(market, isPositive); + v.archived = true; + // solhint-disable-next-line not-rely-on-time + v.activityCheckpoints.unvouchedAt = block.timestamp; + // remove the vouch from the tracking arrays and index mappings + _removeVouchFromArrays(v); + + // apply fees and determine how much is left to send back to the author + (uint256 toWithdraw, ) = applyFees(v.balance, false, v.subjectProfileId); + // set the balance to 0 and save back to storage + v.balance = 0; + // send the funds to the author + // note: it sends it to the same address that vouched; not the one that called unvouch + (bool success, ) = payable(v.authorAddress).call{ value: toWithdraw }(""); + if (!success) { + revert FeeTransferFailed("Failed to send ETH to author"); } - fundsPaid += protocolFee + donation; - - maxPrice = votePrice; - return (votesBought, fundsPaid, votePrice, protocolFee, donation, minPrice, maxPrice); + emit Unvouched(v.vouchId, v.authorProfileId, v.subjectProfileId); } - ``` - -As shown in the snippet, the amount of votes bought is determined by a loop that will run while the `fundsAvailable` are greater than the `votePrice`. As the price of votes increases due to the buy pressure, the loop will be finished, having `fundsAvailable` (which consists of the user's submitted `funds` with the fees substracted) be nearly always greater than the actual `fundsPaid` (which consists of the price paid for each vote + fees + donation fee). - -The problem with this approach is that fees are being applied to an amount that, as demonstrated, is not necessarily the total amount used to actually buy the votes. - - - - +[contracts/contracts/EthosVouch.sol#L452](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/EthosVouch.sol#L452) ### Internal pre-conditions -None +_No response_ ### External pre-conditions -None +_No response_ ### Attack Path -Let's say a user 1 wants to buy 2 `TRUST` votes for a recently created market, where `basePrice` is 0,01 ETH and theres 1 `TRUST` and 1 `DISTRUST` vote already in the market. - -1. The price of one vote is given by `market.votes[isPositive ? TRUST : DISTRUST] * market.basePrice) / totalVotes`, so the price is 0,005 ETH (1 * 0,01 / 2) for the first vote, and ≈ 0,0066 (2 * 0,01 / 3) for the second vote, which adds up to a total of 0,0116 ETH. A 10 % fee is applied (so fee is 0,00116), so the total that user should deposit is 0,01276. - -2. At the same time, and prior to user 1 buying the votes, user 2 submits a buy transaction for one `TRUST` vote. This leaves the state of the vote prices to a different price than the expected by user 1. Still, user 1 submits the transaction with a value of 0,01276. -3. Because user 2 has triggered the buy operation prior to user 1, the initial vote price for user 1 is 0,0066 ETH (2 * 0,01 / 3). As user 1 submitted 0,01276 as `funds`, and without the 10% in fees, the `fundsAvailable` for user 1 are 0,011484. Note that 0,001276 are paid in fees. -4. While in the loop: -- First iteration: `votePrice` starts at 0,0066, and `fundsAvailable` are 0,011484, so one vote is purchased. The `fundsPaid` increases to 0,0066. -- Second iteration: `votePrice` now is at 0,0075 (3 * 0,01 / 4). `fundsAvailable` are 0,004884, so it is not enough to buy a second vote -5. The result is that user 1 has only been able to purchase a single vote. The actual transacted value has only been 0,0066 (the price of one vote), for which a 10% fee would have implied 0,00066 ETH (≈ 2,442 USD). However, as shown in step 1, user 1 has paid 0,00116 ETH (≈ 4,292 USD, **nearly the double!**). - -On the long run, situations like this will arise, leading to a perpetual loss of funds for protocol users due to the increase +Accused profile sees that a lot of complains going against him and unvouch all vouched funds before slashing ### Impact -Medium. As shown in the "Attack Path" section, fees will be overcharged for users that buy votes. On the long term, it is easy that the total value loss exceeds 10 USD or 10% of user value, (considering that new markets might also be configured, with base prices of 0,1 or even 1 ETH). - +authorProfileId can avoid being slashed ### PoC @@ -640,57 +732,29 @@ _No response_ ### Mitigation -When triggering `buyVotes`, allow users to specify the amount of votes they want, instead of the amount of ETH to pay (similar to the logic used when selling). Then, apply the corresponding fees considering the actual amount that user will pay. - -# Issue M-2: Wrong rounding in `_calcVotePrice` will lead to insolvency +```diff -Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/376 ++ function pauseActions(uint authorProfileId) external onlyOwner{ ++ ... ++ } -## Found by -X12, bughuntoor -### Summary -A vote's price gets calculated by the following formula + function unvouch(uint256 vouchId) public whenNotPaused nonReentrant { ++ uint256 authorProfileId = IEthosProfile( ++ contractAddressManager.getContractAddressForName(ETHOS_PROFILE) ++ ).verifiedProfileIdForAddress(msg.sender); ++ require(!isActionsPaused(authorProfileId), "actions paused") + Vouch storage v = vouches[vouchId]; + _vouchShouldExist(vouchId); + _vouchShouldBePossibleUnvouch(vouchId); -```solidity - function _calcVotePrice(Market memory market, bool isPositive) private pure returns (uint256) { - uint256 totalVotes = market.votes[TRUST] + market.votes[DISTRUST]; - return (market.votes[isPositive ? TRUST : DISTRUST] * market.basePrice) / totalVotes; - } ``` -As we can see the vote's price is rounded down (due to Solidity's built-in rounding down). Considering the formula calculates the true price of a vote, the actual paid one would be just slightly lower. Given enough bought votes, rounding down will be enough wei. - -Then, depending on the order they're sold, the rounding down on each step, might be lower. This would cause more funds to be sent during sales, than were taken initially. This would then break the following invariant from the ReadMe -> They must never pay out the initial liquidity deposited. - -Note: currently, the bonding curve formula is flawed, so issue cannot be showcased on itself. However, the formula being broken and not rounding up are two separate issues, hence why I've reported them as such. - - -### Root Cause - -Rounding in wrong direction - -### Attack Path -1. User buys a certain combination of T/D votes in a certain order. Due to rounding down they purchase them for 5 wei less than the true price -2. User then sells them in a different order, due to which, less rounding down occurs, therefore they're sold at higher price than the one bought at. -3. A few wei of market's initial liquidity is distributed to users. - -### Impact -Distributing initial liquidity. - -### Affected Code -https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L922 - -### Mitigation - -Round up within the vote's price formula - -# Issue M-3: Missing slippage protection on `sellVotes()` +# Issue M-2: Missing slippage protection on `sellVotes()` Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/451 ## Found by -0xAnmol, 0xDemon, 0xMosh, 0xaxaxa, 1337web3, Al-Qa-qa, BengalCatBalu, Contest-Squad, DenTonylifer, HOM1T, John44, KlosMitSoss, Kyosi, LeFy, Ryonen, X12, blutorque, bughuntoor, cryptic, dobrevaleri, durov, iamthesvn, immeas, justAWanderKid, ke1caM, mahdikarimi, nikhilx0111, pashap9990, qandisa, redbeans, rscodes, smbv-1923, t.aksoy, t0x1c, tmotfl, underdog, zeGarcao, zhenyazhd, zxriptor +0xAnmol, 0xDemon, 0xMosh, 0xaxaxa, 1337web3, Abhan1041, Al-Qa-qa, BengalCatBalu, Contest-Squad, DenTonylifer, HOM1T, John44, KlosMitSoss, Kyosi, LeFy, Ryonen, X12, blutorque, bughuntoor, cryptic, dobrevaleri, durov, iamthesvn, immeas, justAWanderKid, ke1caM, mahdikarimi, nikhilx0111, pashap9990, qandisa, redbeans, rscodes, smbv-1923, t.aksoy, t0x1c, tmotfl, underdog, zeGarcao, zhenyazhd, zxriptor ### Summary The `ReputationMarket` contract provides preview functions ([simulateBuy()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L761) and [simulateSell()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L806)) to estimate outcomes before actual transactions ([buyVotes()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L442) and [sellVotes()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L495)). While [buyVotes()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L442) includes slippage protection against price changes between simulation and execution, [sellVotes()](https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L495) lacks this safeguard. While Base L2's private mempool prevents traditional frontrunning, users are still exposed to two risks: @@ -729,7 +793,17 @@ _No response_ Implement a slippage control that allows the users to revert if the amount they received is less than the amount they expected. -# Issue M-4: Separate calculation of fees in applyFees results in inflated total fee percentage. + + +## Discussion + +**sherlock-admin2** + +The protocol team fixed this issue in the following PRs/commits: +https://github.com/trust-ethos/ethos/pull/2214 + + +# Issue M-3: Separate calculation of fees in applyFees results in inflated total fee percentage. Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/637 @@ -809,72 +883,3 @@ The protocol team fixed this issue in the following PRs/commits: https://github.com/trust-ethos/ethos/pull/2243 -# Issue M-5: Market Configuration Index Inconsistency - -Source: https://github.com/sherlock-audit/2024-11-ethos-network-ii-judging/issues/732 - -## Found by -0x0x0xw3, 0xShoonya, 0xpiken, Al-Qa-qa, Artur, Ch\_301, Drynooo, HOM1T, MoonShadow, Pheonix, befree3x, bughuntoor, danilych, durov, kutugu, qandisa, rscodes, t0x1c, volodya, xKeywordx -### Summary - -The `removeMarketConfig` function introduces an inconsistency by swapping the last configuration in the array with the one being removed. This behavior disrupts the expected indexing of configuration parameters, leading to the creation of markets with unexpected settings when users rely on specific indices. - -### Root Cause - -When a configuration is removed, the function replaces the targeted index with the configuration at the end of the array and then removes the last element: -https://github.com/sherlock-audit/2024-11-ethos-network-ii/blob/57c02df7c56f0b18c681a89ebccc28c86c72d8d8/ethos/packages/contracts/contracts/ReputationMarket.sol#L403-L406 -```js -function removeMarketConfig(uint256 configIndex) public onlyAdmin whenNotPaused {//checked - // Cannot remove if only one config remains - if (marketConfigs.length <= 1) { - revert InvalidMarketConfigOption("Must keep one config"); - } - - // Check if the index is valid - if (configIndex >= marketConfigs.length) { - revert InvalidMarketConfigOption("index not found"); - } - - emit MarketConfigRemoved(configIndex, marketConfigs[configIndex]); - - // If this is not the last element, swap with the last element - uint256 lastIndex = marketConfigs.length - 1; - if (configIndex != lastIndex) { -@> marketConfigs[configIndex] = marketConfigs[lastIndex]; - } - // Remove the last element -@> marketConfigs.pop(); - } -``` -This index swap results in configurations being reordered, breaking the correspondence between indices and their original parameter sets. Users interacting with `createMarketWithConfig(configIndex)` may unintentionally create markets using unexpected configurations. - - - -### Internal pre-conditions - -N/A - -### External pre-conditions - -_No response_ - -### Attack Path - -1. There are 3 configs -2. Admin removes config at index 1 -3. user create market with configIndex=1 - -### Impact - -Markets could be created with unintended initial parameters - -### PoC - -N/A - -### Mitigation - -To address this issue, avoid swapping configurations when removing an entry. Instead: - -Use an ordered deletion mechanism that retains the array's structure. -