sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

moneyversed - Discrepancy in `_fundPool` fee calculations allowing possible misappropriation of funds due to absence of consistency check between `_amount`, `feeAmount`, and `amountAfterFee` #164

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

moneyversed

high

Discrepancy in `_fundPool` fee calculations allowing possible misappropriation of funds due to absence of consistency check between `_amount`, `feeAmount`, and `amountAfterFee`

An attacker can drain funds from the Allo.sol contract by exploiting the fee mechanism in the _fundPool function.

Vulnerability Detail

The contract is derived from multiple other contracts and libraries such as Ownable, AccessControl, ReentrancyGuardUpgradeable, among others. The purpose of this contract appears to be to manage various investment pools and their associated strategies. These pools can be funded, allocated, and distributed based on the specific strategies.

A key function of interest is _fundPool, which takes an _amount, a _poolId, and a strategy as parameters. This function is responsible for calculating the fee and subsequently transferring the appropriate amounts to the correct destinations.

The flaw

The vulnerability arises within the _fundPool function's fee calculation logic:

if (percentFee > 0) {
 feeAmount = (_amount * percentFee) / getFeeDenominator();
 amountAfterFee -= feeAmount;

 _transferAmountFrom(_token, TransferData({from: msg.sender, to: treasury, amount: feeAmount}));
}

The code calculates feeAmount based on the inputted _amount and a defined percentFee, but notably lacks any verification to ensure the total funds (i.e., the sum of feeAmount and amountAfterFee) correspond with the initial _amount.

Solidity's integer division truncates any remainder, so when the calculation for feeAmount is performed, any discrepancy caused by the truncation is not accounted for. This allows for situations where the sum of the feeAmount and the amountAfterFee could possibly be less than the original _amount, which would mean more funds than necessary could be transferred, exposing the contract to possible fund misappropriations.

In other words, the absence of a check after fee calculation is the root cause, and the code does not guarantee that amountAfterFee + feeAmount = _amount. This is what makes the attack vector viable.

Furthermore, the use of the nonReentrant modifier in the external fundPool (which is calling the internal _fundPool) would prevent reentrancy attacks. However, it is worth to note that this vulnerability does not rely on reentrancy. An attacker could still exploit the described vulnerability by making individual, separate calls to the fundPool function.

An attacker can leverage this vulnerability by choosing specific _amount values for which the truncation discrepancy is maximized. The attacker would then fund the pool repeatedly with these specifically chosen values, slowly draining more funds from the contract than intended.

Impact

An attacker, with enough gas, can repeatedly fund a pool with carefully chosen amounts to drain the contract's funds over time.

Code Snippet

    function fundPool(uint256 _poolId, uint256 _amount) external payable nonReentrant {
        // if amount is 0, revert with 'NOT_ENOUGH_FUNDS()' error
        if (_amount == 0) revert NOT_ENOUGH_FUNDS();

        // Call the internal fundPool() function
        _fundPool(_amount, _poolId, pools[_poolId].strategy);
    }

    function _fundPool(uint256 _amount, uint256 _poolId, IStrategy _strategy) internal {
        uint256 feeAmount;
        uint256 amountAfterFee = _amount;

        Pool storage pool = pools[_poolId];
        address _token = pool.token;

        if (percentFee > 0) {
            feeAmount = (_amount * percentFee) / getFeeDenominator();
            amountAfterFee -= feeAmount;

            _transferAmountFrom(_token, TransferData({from: msg.sender, to: treasury, amount: feeAmount}));
        }

        _transferAmountFrom(_token, TransferData({from: msg.sender, to: address(_strategy), amount: amountAfterFee}));
        _strategy.increasePoolAmount(amountAfterFee);

        emit PoolFunded(_poolId, amountAfterFee, feeAmount);
    }

(public) fundPool-Allo.sol#L339-L345

(internal) _fundPool-Allo.sol#L509-L514

Tool used

Manual review.

Recommendation

Implement a verification step after the fee calculation within the _fundPool function:

require(_amount == feeAmount + amountAfterFee, "Invalid amount calculations");

This check ensures that the sum of feeAmount and amountAfterFee always corresponds to the initial _amount, effectively safeguarding against unintended transfers.

Alternatively, consider using a more precise method of fee calculation or incorporating rounding mechanisms that can account for the discrepancy caused by truncation, ensuring that the correct amount is always transferred.

POC

The idea here is to maximize the amount that is "left out" when calculating feeAmount. The feeAmount is calculated as follows:

feeAmount = (_amount * percentFee) / getFeeDenominator();

If _amount * percentFee is not a multiple of getFeeDenominator(), the Solidity division will truncate the result, causing a small amount of value to be lost. This is the amount we wish to maximize to exploit the contract optimally.

The maximum discrepancy is introduced when _amount * percentFee is one less than a multiple of getFeeDenominator(). In mathematical terms:

(_amount * percentFee) % getFeeDenominator() = getFeeDenominator() - 1

Solving for _amount gives us:

_amount = ((getFeeDenominator() - 1) + (getFeeDenominator() * n)) / percentFee

Here, n is any non-negative integer. The choice of n would depend on how much _amount you can invest.

One could create a loop that calculates the optimal value of n iteratively, ensuring that the discrepancy is exploited to its maximum.

const { ethers } = require("hardhat");

async function main() {
// Initialize and deploy contracts
const [deployer] = await ethers.getSigners();
console.log("Deploying contracts with the account:", deployer.address);

// Assume other required contracts are deployed, such as registry, etc.
const REGISTRY_ADDRESS = "0xRegistryAddress";
const STRATEGY_ADDRESS = "0xStrategyAddress";
const TOKEN_ADDRESS = "0xTokenAddress";

const Allo = await ethers.getContractFactory("Allo");
const allo = await Allo.deploy(REGISTRY_ADDRESS, deployer.address);  // Treasuring being the deployer for example
await allo.deployed();

// Fetch percentFee and baseFee from the deployed contract
const percentFee = await allo.percentFee();
const baseFee = await allo.baseFee();

// Create a pool
const PROFILE_ID = ethers.utils.formatBytes32String("exampleProfile");
const INIT_STRATEGY_DATA = ethers.utils.toUtf8Bytes("exampleData");
const METADATA = {
    name: "example pool",
    description: "example description",
    extra: "Any extra data we want to declare"
};
const MANAGERS = [deployer.address];

const poolId = await allo.createPool(
    PROFILE_ID,
    STRATEGY_ADDRESS,
    INIT_STRATEGY_DATA,
    TOKEN_ADDRESS,
    baseFee, // Using baseFee as an example amount
    METADATA,
    MANAGERS
);

// Determine the fee structure
const feeDenominator = await allo.getFeeDenominator();

// Craft the amount to exploit the fee discrepancy
const MAX_ITERATIONS = 100;  // Arbitrarily chosen. Adjust as needed.

for (let i = 0; i < MAX_ITERATIONS; i++) {
    const chosenAmount = ((feeDenominator.sub(1)).add(feeDenominator.mul(i))).div(percentFee);

    await allo.fundPool(poolId, chosenAmount);

    // Now, let's check the discrepancy
    const strategyAddress = (await allo.pools(poolId)).strategy;
    const strategyBalance = await ethers.provider.getBalance(strategyAddress);
    const expectedBalance = chosenAmount.sub((chosenAmount.mul(percentFee)).div(feeDenominator));

    console.log(`Iteration ${i}`);
    console.log("Expected balance in strategy:", expectedBalance.toString());
    console.log("Actual balance in strategy:", strategyBalance.toString());

    if(!expectedBalance.eq(strategyBalance)) {
        console.error("Discrepancy found in iteration ", i);
    }
}

main().catch(error => {
    console.error(error);
    process.exit(1);
});

In this script:

We start by setting up the environment.
We fetch the feeDenominator and percentFee from the contract itself.
A formula is used to calculate chosenAmount that maximizes the discrepancy due to Solidity's truncating division. This chosenAmount will be used to fund the pool.
We repeatedly fund the pool with the chosenAmount and then check the strategy's balance.
The actual balance in the strategy and the expected balance are then compared. If they don't match, it confirms the vulnerability.

The script leverages the flaw by calculating an _amount which maximizes the truncation discrepancy:

(_amount * percentFee) % getFeeDenominator() = getFeeDenominator() - 1

By carefully choosing MAX_ITERATIONS (n times) and executing this repeatedly using different values of n, an attacker could exploit this vulnerability to the maximum extent, thus eventually draining the contract.

sherlock-admin commented 1 year ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, don't see how funds could be drained

moneyversed commented 1 year ago

Considering the following:

https://gist.github.com/moneyversed/1cef92c69184b82fac9cfec83ce13117

don't see how funds could be drained

In the gist above we managed to achieve the required environment, set a custom strategy, deploy the necessary CAs, and calculate the most fitting _amount (chosenAmount) to fund the pool in a manner that maximizes the truncation discrepancy.

The script then funds the pool with this calculated amount, checks the actual balance transferred to the strategy, and compares it with the expected balance after fee deduction.

Further specifically, the provided output confirms the vulnerability:

Expected balance in strategy: 900000000000000009
Actual balance in strategy: 1980000000000000036

LINK TO STRATEGY (deployed on Goerli);

LINK TO `allocate` TX STATE CHANGE;

LINK TO `createPoolWithCustomStrategy` TX LOG;

LINK TO `fundPool` TX LOG.

For the entire version including all the respective logs, see the linked gist.

The discrepancy between the expected and actual balances actually indicates that more funds than intended are being transferred due to the truncation vulnerability, which is consistent with my report. Though we didn't even need an iterative loop mechanism nor a more complex exploit, as i do think it is nonetheless quite evident as of the current impl.

From the output, we can deduce:

The fee mechanism in the contract is overly flawed, as the actual balance transferred to the strategy after fee deduction is more than what is intended.
The discrepancy is due to Solidity's truncation behavior, which results in the sum of the fee and the amount after fee being less than the original amount.
This vulnerability can be exploited by attackers who carefully choose amounts that maximize the truncation discrepancy.

Where drained_amount = actual_balance - expected_balance; we'd then calculate the percentage drained relative to the expected balance: *percentage_drained = (drained_amount / expected_balance) 100 = 120.0**

To make it as clear as possible there also is a live video proof of the exploit by the end of the provided gist, alongside with the deployment, pool creation, funding and other related txns.

According to the above the CA was drained by an additional 120% of the expected balance due to the vulnerability. This means that, for every unit of token intended to be in the CA, an additional 1.2 units were added due to the exploit.

a12jmx commented 1 year ago

Escalate I think this issue has been actually misjudged as for the above comment and the provided evidence (see mentioned gist), which'd in turn make this a valid high severity concern.

The point though is that anyone can create a custom strategy, meaning that by calculating a specific amount (and eventually funding the pool throughout multiple iterations), anyone can game the system into funding more than they should get post-fee deduction.

Furthermore, in the script:

1. A specific amount is calculated that takes into account the fee deductions, such that post-deduction the attacker ends up with the desired amount.
1. This specific amount is deposited into the pool.
1. Upon funding (either individually/lower impact or repeatedly/higher impact as provided in the gist), the system deducts the fees and transfers the rest to the strategy.
1. Due to the way the amount was calculated, post-deduction the attacker gets way more than they should have (in their strategy).

This condition for instance could be exploited to gain an unfair advantage either would likely have financial consequences for the protocol and its users and, if it was to be exploited on a large scale, would lead to huge losses. For instance, if we consider a system processing millions of dollars in txns, those "tiny" discrepancies can eventually add up to significant amounts, especially if the pool is repeatedly funded 369 different times as per the additional logic provided by the end of the gist, thus making the outcome evidently quite extensive on the attacker side.

As for the output where fundPool is repeatedly called by the end of the linked gist, we can observe the following output logs:

Expected balance in strategy: 990000000000000000

Actual balance in strategy: 402930000000000003330

Quoting the above comment on the formula:

*Where drained_amount = actual_balance - expected_balance; we'd then calculate the percentage drained relative to the expected balance: percentage_drained = (drained_amount / expected_balance) 100 = 40600.0**

This indicates that the strategy managed to accumulate an enormous (way more concrete than the individual funding behavior.) discrepancy of an additional 40,600% (differently from the previous output) compared to the expected balance, providing actual evidence that the vulnerability exists and is exploitable by anyone as far as they were to setup a custom strategy and perform the necessary steps in order to create and fund the pool through passing the most proper chosenAmount to the CA (that'd make the paid fees to reflect a smaller amount than how much he's actually profiting from the exploit), thus being able to accumulate a solid profit within their strategy overtime.

Escalator: moneyversed

sherlock-admin2 commented 1 year ago

Escalate I think this issue has been actually misjudged as for the above comment and the provided evidence (see mentioned gist), which'd in turn make this a valid high severity concern.

The point though is that anyone can create a custom strategy, meaning that by calculating a specific amount (and eventually funding the pool throughout multiple iterations), anyone can game the system into funding more than they should get post-fee deduction.

Furthermore, in the script:

A specific amount is calculated that takes into account the fee deductions, such that post-deduction the attacker ends up with the desired amount.

This specific amount is deposited into the pool.

Upon funding (either individually/lower impact or repeatedly/higher impact as provided in the gist), the system deducts the fees and transfers the rest to the strategy.

Due to the way the amount was calculated, post-deduction the attacker gets way more than they should have (in their strategy).

This condition for instance could be exploited to gain an unfair advantage either would likely have financial consequences for the protocol and its users and, if it was to be exploited on a large scale, would lead to huge losses. For instance, if we consider a system processing millions of dollars in txns, those "tiny" discrepancies can eventually add up to significant amounts, especially if the pool is repeatedly funded 369 different times as per the additional logic provided by the end of the gist, thus making the outcome evidently quite extensive on the attacker side.

As for the output where fundPool is repeatedly called by the end of the linked gist, we can observe the following output logs:

Expected balance in strategy: 990000000000000000

Actual balance in strategy: 402930000000000003330

Quoting the above comment on the formula:

*Where drained_amount = actual_balance - expected_balance; we'd then calculate the percentage drained relative to the expected balance: percentage_drained = (drained_amount / expected_balance) 100 = 40600.0**

This indicates that the strategy managed to accumulate an enormous (way more concrete than the individual funding behavior.) discrepancy of an additional 40,600% (differently from the previous output) compared to the expected balance, providing actual evidence that the vulnerability exists and is exploitable by anyone as far as they were to setup a custom strategy and perform the necessary steps in order to create and fund the pool through passing the most proper chosenAmount to the CA (that'd make the paid fees to reflect a smaller amount than how much he's actually profiting from the exploit), thus being able to accumulate a solid profit within their strategy overtime.

Escalator: moneyversed

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0x00ffDa commented 1 year ago

So, this is basically paying a reduced fee(?)

moneyversed commented 1 year ago

Though it doesn't just stop at paying a reduced fee. @0x00ffDa

The point though is that anyone can create a custom strategy, meaning that by calculating a specific amount (and eventually funding the pool throughout multiple iterations), anyone can game the system into funding more than they should get post-fee deduction.

This states a clear and direct financial gain at the expense of the protocol and legit users.

Ain't just about one user making a single tx and saving on fees. It's about the risk for an attacker to repeatedly exploit this, accumulating boundless gains over time due to this discrepancy. As such, if we look at the provided script, the attacker funded the pool 369 different times, each time exploiting this vector, leading to a snowball effect. Meaning the more is the volume which the system processes, the wider will be the room for this discrepancy to eventually accumulate and disrupt the Allo over time. Further, with the ability to fund the pool repeatedly (as shown in the provided script), by also keeping particular attention on passing a proper fee / amount setup so that the outcome will be bigger and actually worth whatever is initially spent, the attacker can accumulate quite an abnormal discrepancy. The output showed that the strategy managed to accumulate an additional 40,600% discrepancy / 406x more than the expected balance, which in turn means he gained +406x times the amount he would've been able to obtain if the protocol was safe. This would further increase as for each time that the pool is subsequently funded. (i.e., assume we were to fund the pool 3690 times instead of 369 throughout the same setup, the accumulated gap in the strategy would then be about a 405,910% / 4059.1x more compared to the expected balance given the formula (4019499000000000033219 (isActualBalance) - 990000000000000000 (isExpectedBalance)) = 4018509000000000033219 (isDrainedDifference) / 990000000000000000 (isExpectedBalance) * 100 = 405,910.)

The fact that anyone can set up a custom strategy and exploit this behavior raises its severity. It's not a closed system where only a few privileged actors have this ability. This means the threat surface is broad, and the risk of exploitation is kind of high, so while your surface-level description of "paying a reduced fee" may seem to be overlooking on the claimed impact, the consequences and implications of this vulnerability are undoubtedly severe. It's not just about a fee discrepancy but about the room for unlimited financial gain by malicious actors at the expense of the platform and its users.

neeksec commented 1 year ago

Suggest to keep invalid.

Expected balance in strategy: 900000000000000009 Actual balance in strategy: 1980000000000000036

Actual balance in strategy > Expected balance in strategy

The truncating division enable pool mananager to bypass protocol fee and this profits the strategy. I don't see which smart contract was drained. And bypassing protocol fee is an acceptable risk.

moneyversed commented 1 year ago

The truncating division enable pool mananager to bypass protocol fee and this profits the strategy. I don't see which smart contract was drained. And bypassing protocol fee is an acceptable risk.

What the judge is stating here makes no sense at all and quite trying to misguide considering what has been already demonstrated.

For instance, why would a protocol include protocol fees if they were meant to be eventually bypassed? If a protocol outlines a fee structure, it's meant to be enforced because in any system, especially in financial ones, fees are used to maintain protocol's sustainability, fund development, or reward certain participants (like stakers, lp providers, etc.). By allowing a mechanism to bypass these, the system though not only loses pending revenue but also opens a door to attacks that could drain funds or exploit the protocol in unintended ways. Bypassing it, especially in a way that profits certain actors, I think goes against the intended design. It's akin to a bank saying they charge fees for certain services but then having a mechanism where clever customers can avoid them while profiting.

Furthermore, as demonstrated in my previous comment the attacker is able to make immeasurable profit upon this vector through calling it recursively (this further amplifies the actual damage, making it an even more severe threat.), not just a 2x discrepancy as showed in the 1st output, but rather in 2nd and 3rd example I've demonstrated that the updated setup manages to achieve quite an evident costOfFees < realizedProfit gap, thus he can gain huge differences like 400 Xs or even 4000 Xs the expected amount, allowing far more funds than intended to be moved to the strategy, which in turn isn't a minor rounding error but a serious gap that can result in sizeable financial losses for the protocol and its users. This is basically a drain from the main CA to the attacker's strategy. Over time and with repeated exploitation, such a behavior can lead to massive losses, moreover if dealing with large amounts of funds means though the discrepancies may seem small individually, when exploited reateadly they can accumulate to significant amounts, especially in high-volume systems.

The judge's dismissal of it as merely an accettable risk and not acknowledging the eventuality for recursive exploitation seems to overlook the core issues and the evidence provided. Once the attacker accumulated the profited discrepancy in his custom strategy, he would basically add a withdraw function within the strategy's code and redeploy the CA in order to transfer the outcomes at their own personal address, simple as that, though would still result in catastrophic consequences. So I do actually stand up with what I've been saying in the comments and escalation up to this time and will be waiting for Sherlock's final acknowledgement.

MLON33 commented 1 year ago

https://github.com/allo-protocol/allo-v2/pull/385

Evert0x commented 1 year ago

@neeksec any follow up comment? I don't see why bypassing protocol fee is an acceptable risk. It's a loss of funds to the protocol instead fees are not collected as expected.

neeksec commented 1 year ago

I said bypassing protocol fee is an acceptable risk is because,

Fee skirting where pool manager directly fund the pool without paying the fees

This is a known/acceptable issue. I think any method to bypass fee that is more complex than this should also be acceptable. Since there is an acceptable easier way.

The recommandation is misleading.

require(_amount == feeAmount + amountAfterFee, "Invalid amount calculations");

This will never revert.

        uint256 amountAfterFee = _amount;
        ....

        if (percentFee > 0) {
            feeAmount = (_amount * percentFee) / getFeeDenominator();
            amountAfterFee -= feeAmount;
       ....
+     require(_amount == feeAmount + amountAfterFee, "Invalid amount calculations");

This one led sponsor @0xKurt to produce a needless fix https://github.com/allo-protocol/allo-v2/pull/385/commits/145fba7d826d54c6eb569c44f41e8d8fec1f59e7.

moneyversed commented 1 year ago

Extremely disagree with the following assumptions:

Fee skirting where pool manager directly fund the pool without paying the fees I think any method to bypass fee that is more complex than this should also be acceptable. Since there is an acceptable easier way.

As I've managed to already explain and thoroughly demonstrate why this is actually a risk to the protocol and how to exacerbate the whole attack even more, being able to profit up to infinite % of discrepancies within the attacker's strategy. So with all due honesty, I still don't really get how comes in your mind that makes you think of this as nothing but "an accettable risk".

To mention a pretty recent exploit that happened only a few days ago which was quite similar to the scenario I've provided, see the following thread:

https://twitter.com/octane_security/status/1716336906097885601

Where a considerable amount of funds (+528 ether) has been lost due to precision loss issues, along with manipulation of the liquidityIndex (which was an interested indicator of that specific protocol.), proving that truncating fees and precision loss could eventually end up in far worse behavior whether it is not being handled properly. As a result, I don't have much more to argue on the considerability/profitability of such a scenario and will just keep standing with my responses and escalation above, as I still tend to consider this as a valid high-priority concern based on what has been mentioned prior to this moment.

Though nonetheless, yet I think you're likely right on the fact that the "require(_amount == feeAmount + amountAfterFee, "Invalid amount calculations");" check is currently not sufficient by itself to prevent this vector, but you're inaccurate in stating that

This will never revert

As we mainly don't want the _fundPool to revert, but rather to handle the fees properly. Specifically, a more pertinent and on point prop rather than the initial check that @0xKurt managed to implement would be to update the _fundPool's code as follows:

function _fundPool(uint256 _amount, uint256 _poolId, IStrategy _strategy) internal {
 if (_amount == 0) revert NOT_ENOUGH_FUNDS();
 require(_amount >= MINIMUM_AMOUNT, "Amount too small");

 Pool storage pool = pools[_poolId];
 address _token = pool.token;

 uint256 feeAmount;
 uint256 amountAfterFee = _amount;

 if (percentFee > 0) {
 uint256 feeDenominator = getFeeDenominator();
 require(feeDenominator > 0, "Invalid fee denominator");

 feeAmount = (_amount * percentFee + feeDenominator - 1) / feeDenominator;
 amountAfterFee = _amount - feeAmount;

 require(_amount == feeAmount + amountAfterFee, "Invalid amount calculations");

 _transferAmountFrom(_token, TransferData({from: msg.sender, to: treasury, amount: feeAmount}));
 }

 _transferAmountFrom(_token, TransferData({from: msg.sender, to: address(_strategy), amount: amountAfterFee}));
 _strategy.increasePoolAmount(amountAfterFee);

 emit PoolFunded(_poolId, amountAfterFee, feeAmount);
}

Where the fee calculation this time is adjusted to round up instead of truncating, minimizing the eventual discrepancy:

$`\text{feeAmount} = \frac{{\text{\_amount} \times \text{percentFee} + \text{feeDenominator} - 1}}{{\text{feeDenominator}}}`$

Also, a further requirement is added to ensure that the amount being funded is above a certain minimum (where MINIMUM_AMOUNT should be declared accordingly.), which provides an additional layer of validation.

require(_amount >= MINIMUM_AMOUNT, "Amount too small");

Lastly, I do sincerely want to apologize and admit I should've provided a fully working fix in first place instead of waiting this far.

neeksec commented 1 year ago

Let's assume Allo charges 10% protocol fee(to coordinate with the report). getFeeDenominator() constantly returns 1e18 and percentFee will be 1e17 .

feeAmount = (_amount * percentFee) / getFeeDenominator();

In the original equation. The maxmuim feeAmount loss will be 1wei per call because of rounding down. I don't think lossing 1wei of fee each call is a big deal, no matter how many calls has been made.

This gist (https://gist.github.com/moneyversed/1cef92c69184b82fac9cfec83ce13117) provides two PoCs and both have log errors.

In the first one,

Base Fee: 10

Total amount with fee: 1100000000000000019

Percent Fee: 100000000000000000

Fee Denominator: 1000000000000000000

Calculated Fee Amount: 100000000000000000

Amount After Fee Deduction: 900000000000000009

mockERC20 is a valid contract too.

Amount for pool: 1100000000000000019

Allo contract allowance: 1000000000000000000000000000000000000000 - LINK TO APPROVAL TX LOG;

Deployer's MockERC20 balance before transfer: 1000000000000000000000.0

Initial Strategy MockERC20 balance: 0.0

Pool Created: 1 - LINK TO createPoolWithCustomStrategy TX LOG;

Pool funded successfully. - LINK TO fundPool TX LOG;

Discrepancy found!

Exploit triggered! - LINK TO allocate TX STATE CHANGE;

Expected balance in strategy: 900000000000000009

Actual balance in strategy: 1980000000000000036

The pool was funded one time with token amount 1100000000000000019 and says Expected balance in strategy: 900000000000000009. But the log says Actual balance in strategy: 1980000000000000036 which is bigger than the orginal funded amount(1100000000000000019). This means there are already balances sent in strategy before fundPool.

This extra balance was sent during strategy creation, so Actual balance in strategy should not be compared with Expected balance in strategy.

 try {
 const poolTx = await allo.createPoolWithCustomStrategy(
 PROFILE_ID,
 strategy.address,
 INIT_STRATEGY_DATA,
 mockERC20.address,
 amountForPool,
 METADATA,
 MANAGERS,
 {
 value: valueToSend,
 gasLimit: 6000000 // Increase gas limit (Adjust accordingly)
 }
 );

In the second PoC,

const targetAmountAfterFee = ethers.utils.parseEther("0.99"); // 990000000000000018

const getFeeDenominator = await allo.getFeeDenominator(); 
const percentFee = await allo.getPercentFee(); 

const baseFee = await allo.getBaseFee();
console.log("Base Fee:", baseFee.toString());

// Rearrange the formula to achieve targetAmountAfterFee:
const chosenAmount = targetAmountAfterFee.mul(getFeeDenominator).div(getFeeDenominator.sub(percentFee));

// Calculate the fee that will be deducted when funding the pool
const feeAmount = (chosenAmount.mul(percentFee)).div(getFeeDenominator);
const totalAmountWithFee = chosenAmount.add(feeAmount).add(baseFee);

// other code prior to pool funding

// Repeatedly fund the pool
const timesToFund = 369; // or any other significant number

for (let i = 0; i < timesToFund; i++) {
await allo.fundPool(poolId, totalAmountWithFee);
}
console.log("Pool funded successfully.");

Where the output would then be:

Base Fee: 10

Total amount with fee: 1210000000000000010

Percent Fee: 100000000000000000

Fee Denominator: 1000000000000000000

Calculated Fee Amount: 110000000000000000

Amount After Fee Deduction: 990000000000000000

Amount for pool: 1210000000000000010

Initial Strategy MockERC20 balance: 0.0

Pool Created: 1

Pool funded successfully.

Discrepancy found!

Exploit triggered!

Expected balance in strategy: 990000000000000000

Actual balance in strategy: 402930000000000003330

1210000000000000010 token was funded 369 times. The 'Expected balance in strategy' should be 1210000000000000010 * 369 * 90% which is 401841000000000003321 not 990000000000000000.

moneyversed commented 1 year ago

If we look at the provided PoCs:

PoC no. 1

In this scenario, the pool is funded with an amount of $1.1 \times 10^{18}$ wei. The expected balance in the strategy after the fee deduction should be $0.9 \times 10^{18}$ wei. However, the actual balance in the strategy is reported as $1.98 \times 10^{18}$ wei, which is more than the original funded amount. Here I believe you're right as I've indeed funded the pool prior to the fundPool tx, and thus the calculation was currently not accurate.

PoC no. 2

In the second scenario, the pool is rather funded 369 times with an amount of $1.21 \times 10^{18}$ wei each time. The expected balance in the strategy after the fee deductions should be (0.99 ETH * 369 = 365.31 ETH) which also equals to 365,310,000,000,000,000,009 wei, so the calculation is wrong even in this case. However, the actual strategy is actually 402,930,000,000,000,003,330 (402.93 ETH) which is still about a 10% profit, which in turn means the attacker can accumulate up to a 10% of profit as for each time that he funds the pool 369 different times. So assume he creates 100 different instances of this specific recursive funding path out of the same provided setup (369 times for each instance), he would have a 1000% / +10x profit within his own strategy* as 10% 100 instances = 1000%, which is more than a single wei of loss per call by far**. And that's a single individual, means that if multiple individuals exploits this vector at the same time losses would be quite drastic.

Moreover, the calculation you've mentioned,

The 'Expected balance in strategy' should be 1210000000000000010 369 90% which is 401841000000000003321 not 990000000000000000.

would not be accurate in this context because:

The 90% factor seems to be a misunderstanding since the actual amount after fees is 99% of 1 ether, not 90%.
The amountForPool (1210000000000000010) includes fees and the base fee, which should not be part of the amount invested in the strategy and therefore should not be multiplied directly.

Specifically:

The Allo CA calculates the fee based on the amount sent and deducts it.
If the base fee is greater than zero, it is also deducted.
The remaining amount (net amount) is sent to the strategy and recorded.

So, in the script, by calling fundPool(poolId, totalAmountWithFee), we are sending the total amount including fees and the base fee. The Allo then deducts the fees and the base fee, and thus only the net amount is invested in the strategy.

For instance, for the calculations to be properly set we should adjust the expectedBalance within the script to reflect 0.99 (targetAmountAfterFee) * 369 (timesToFund):

const strategyBalanceAfterFunding = await mockERC20.balanceOf(strategy.address);
const expectedBalance = initialStrategyBalance.add(targetAmountAfterFee.mul(timesToFund));

if(!expectedBalance.eq(strategyBalanceAfterFunding)) {
console.error("Discrepancy found!");

In this case the output would be:

Expected balance in strategy: 366399000000000000009 Actual balance in strategy: 402930000000000003330

Now, to break down the script's calculations:

Total amount with fee for all fundings: $1.21 \, \text{ETH} \times 369 = 446.49 \, \text{ETH}$

This includes the fee for each funding.

Base fee: $0.1 \, \text{ETH}$

This is a one-time fee for initializing the CA.

Total ETH sent to strategy: $446.49 \, \text{ETH} + 0.1 \, \text{ETH} = 446.59 \, \text{ETH}$
Actual balance in strategy after all fundings: $1.1 \, \text{ETH} \times 369 = 405.9 \, \text{ETH}$ / 402.93 ETH after fees;

This is the amount deposited in each funding.

Expected Balance in strategy after all fundings: $0.99 \, \text{ETH} \times 369 = 365.31 \, \text{ETH}$

This is the target amount after the fee is deducted.

The profit per call made by the attacker is actually the difference in what they were able to deposit into the strategy per call, minus what should have been the maximum allowable amount after fees.
Deposited per call: $1.1 \, \text{ETH}$
Maximum allowable after fees: $0.99 \, \text{ETH}$
Profit per Call: $1.1 \, \text{ETH} - 0.99 \, \text{ETH} = 0.11 \, \text{ETH}$ / 0.10198 ETH after fees;

Now, multiplying the profit per call by the number of fundings:

$0.10198 \, \text{ETH} \times 369 = 37.63 \, \text{ETH}$

This exactly matches the discrepancy between the expected and actual balances in the strategy, confirming that the cumulative effect of the exploit over all the fundings is what caused the 37 ETH discrepancy, where the difference between the total ETH sent to the strategy (446.59) ETH) and the actual balance in the strategy after all fundings (402.93 ETH) is the total fee collected by the CA (37.63 ETH) (basically the amount which the attacker managed to profit from the exploit. The base fee (0.1 ETH) is also included in this calculation.). This allows the attacker to deposit more than what should be allowed after fees. The profit per call made by the attacker (0.10198 ETH) contributes to this discrepancy over the 369 fundings.

In general, points raised from @neeksec would be valid in a scenario where the loss due to rounding down is indeed insignificant. However, the above clarification demonstrates that the actual impl of the fee calculation can lead to significant discrepancies, especially when the pool is funded multiple times. So even with these points in mind and despite the flaws in the POCs' scenarios, this still stands as a high-priority concern as per the fact that loss due to rounding down in fee calculations may seem kind of uneffective on a per-call basis, but when the system is subjected to multiple txns followed from large amounts of funds, these losses would accumulate, leading to unbounded discrepancies eventually reaching thousand of ethers of gains. This can be exploited to drain funds from the CA, as demonstrated by the PoCs, which is a critical issue for any financial or defi protocol. Therefore I stand with my initial claims and believe judge's assessment may not have taken into account the cumulative effect of the rounding down over multiple txns, seems to me like a clear underestimation of the vulnerability’s impact.

neeksec commented 1 year ago

The fee lost here because of the rounding won't be larger than 1wei.

feeAmount = (_amount * percentFee) / getFeeDenominator();

I think I've provided enough and can wait for @Evert0x to decide.

moneyversed commented 1 year ago

With all due respect, I honestly don't feel like changing my mind at all upon the main perspective I initially had on this issue, moreover I continue to believe bypassing protocol fee just can't be kind of acceptable behavior as it logically goes against the ethical principles and tolerance of any protocol trying to make an healthy way around there in web3 and even outside of the blockchain, moreover if exploited recursively as explained in the above comment, could eventually lead to huge gaps that should not be disclosed nor in either way allowing for tricky actors to illegitimately profit from them which makes the core issue here about a flaw that overlooks on the scope of granting a secure environment as in regards of users who decide to put their trust and invest their resources into a project, because the current impl of the Allo and the actual handling of protocol fees within the CA seems to be the opposite of "safe" as of the time of this speech.

So I still consider this a solid finding which would likely result in the impact I've claimed within the previous argumentations which is far more serious and a worse concern than you'd like to make it out to be, and since I've already managed to say whatever I had to say, yet I do fully respect your take, though will just be looking forward to a fair and objective resolution on Sherlock's side.

Evert0x commented 1 year ago

The core issue is incorrect rounding in this line

feeAmount = (_amount * percentFee) / getFeeDenominator();

getFeeDenominator() is always 10**18 percentFee is always smaller than 10**18

You will see this type of calculating in every codebase.

There is no issue here.

moneyversed commented 1 year ago

I believe the issue at hand is being significantly underestimated, particularly when drawing parallels to the recent HopeLend protocol exploit, where precision loss vulnerabilities were exploited, resulting in a loss of 528 ETH ($835k). In the HopeLend hack, attackers were able to manipulate the liquidityIndex, causing a distortion in fee calculations that rippled across the entire system. This was not a case of siphoning off 'dust' or lost fractions slowly; it resulted in significant gains in a single tx, allowing the attacker to borrow assets at substantially reduced costs.

In the Allo, the vulnerability rather lies in the rounding down of fee calculations.

feeAmount = (_amount * percentFee) / getFeeDenominator();

While different in its implementation, is not immune to these risks. This seemingly benign line of code, as mentioned by @Evert0x, is indeed a common pattern found in many codebases. However, it is the cumulative effect of precision loss over multiple txns and the risk for exploitation that raises red flags, thus escalating its actual damage. (see https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/164#issuecomment-1783179450). Specifically, the cumulative effect of this rounding down assuming multiple txns can lead to a substantial discrepancy, as demonstrated in the provided PoCs. In the second PoC, where the pool was funded 369 times, the attacker was able to accumulate up to a 10% profit. Extrapolating this, with 100 different instances of this specific funding pattern, the attacker could eventually amass a 1000% profit, demonstrating a significant and exploitable profit gap, likewise to what was witnessed in the HopeLend hack.

This is why I strictly believe this is not just an "acceptable risk"; it has practical implications, as seen in the HopeLend exploit. The precision loss and manipulation of key variables led to a loss of over 528 ether, a basic reminder of the possible financial impact of such vulnerabilities, even if they appear limited at a first glance.

Further, the argument that losing 1 wei of fee per call is not a big deal is a dangerous underestimation. In Defi protocols, where large sums of money are transacted, even the smallest discrepancies can accumulate to substantial amounts. To repeat, the provided PoCs demonstrate scenarios where the attacker can profit up to 10% per instance of the attack, and when replicated across multiple instances, the profit can scale up to 1000%, showcasing a clear and present danger to the protocol’s integrity. Allowing for any form of protocol fee bypass, regardless of the amount, goes against the foundational principles of trust and security that users place in a protocol. The current impl of the Allo, and the handling of protocol fees within the CA, is a stark deviation from these principles.

In light of the above, I firmly stand by my initial claims and urge a reevaluation of the assessed impact and severity of this issue. The parallels drawn from the HopeLend Protocol Hack serve as a critical reminder that precision loss issues, if left unaddressed, can escalate to catastrophic levels.

Evert0x commented 1 year ago

Result: Invalid Unique

After an extended back and forth with the Watson it was unclear how this issue is something else than a insignificant rounding error.

>>> _amount = 1210000000000000010                                 
>>> percentFee = 100000000000000000
>>> getFeeDenominator = 1000000000000000000
>>> amountAfterFee = _amount
>>> feeAmount = (_amount * percentFee) / getFeeDenominator
>>> feeAmount
1.21e+17
>>> amountAfterFee -= feeAmount
>>> amountAfterFee
1.089e+18
>>> int(_amount)
1210000000000000010
>>> int(feeAmount + amountAfterFee)
1210000000000000000
>>>

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

a12jmx: rejected

sherlock-audit / 2023-09-Gitcoin-judging

moneyversed - Discrepancy in `_fundPool` fee calculations allowing possible misappropriation of funds due to absence of consistency check between `_amount`, `feeAmount`, and `amountAfterFee` #164

Discrepancy in _fundPool fee calculations allowing possible misappropriation of funds due to absence of consistency check between _amount, feeAmount, and amountAfterFee

Vulnerability Detail

The flaw

Impact

Code Snippet

Tool used

Recommendation

POC

https://gist.github.com/moneyversed/1cef92c69184b82fac9cfec83ce13117

LINK TO STRATEGY (deployed on Goerli);

LINK TO allocate TX STATE CHANGE;

LINK TO createPoolWithCustomStrategy TX LOG;

LINK TO fundPool TX LOG.

$\text{feeAmount} = \frac{{\text{\_amount} \times \text{percentFee} + \text{feeDenominator} - 1}}{{\text{feeDenominator}}}$

PoC no. 1

PoC no. 2

Discrepancy in `_fundPool` fee calculations allowing possible misappropriation of funds due to absence of consistency check between `_amount`, `feeAmount`, and `amountAfterFee`

LINK TO `allocate` TX STATE CHANGE;

LINK TO `createPoolWithCustomStrategy` TX LOG;

LINK TO `fundPool` TX LOG.

$`\text{feeAmount} = \frac{{\text{\_amount} \times \text{percentFee} + \text{feeDenominator} - 1}}{{\text{feeDenominator}}}`$