dimulski - If all LV tokens are requested for redeem, issuing new CT and DS tokens will revert #114
Comments
|
This is a valid issue, though there's an update in which we removed all the functionality related to the expiry redemption in the LV(all of withdrawal is done through redeemEarly). so won't fix |
sherlock-admin3
added
Sponsor Confirmed
sherlock-admin3
changed the title
|
Escalate The above issue describes about an edge case where all the users are redeeming causing division by 0. |
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. |
This issue has nothing to do with #211, the reason why division by 0 is possible in #211 is because when the redeemEarly() function is called the LV tokens are burned. On the contrary, when the requestRedemption() function is called the LV tokens are not burned. |
|
I agree with @AtanasDimulski comment. This issue is unique Medium. |
|
This issue is low severity because anyone can donate some ra to the uniswap pool so that there is enough liquidity and it does not revert. Hence, the DoS does not hold for 1 week. If the ra amount is 0, it skips adding liquidity, so no problem here either. |
First of all, I want to state that this comment has nothing to do with the original escalation, and shouldn't even be considered but nevertheless let's entertain its validity. The issueNewDs() function, calls the deploySwapAssets() function which deploys the CT and DS tokens, as well as the CT and RA pair, so it is not possible to donate anything to something that doesn't exist. As can be seen from the code snippet below: (address ct, address ds) = IAssetFactory(SWAP_ASSET_FACTORY).deploySwapAssets(
ra, state.info.pair0, address(this), expiry, exchangeRates
);
address ammPair = getAmmFactory().createPair(ra, ct); function deploySwapAssets(address ra, address pa, address owner, uint256 expiry, uint256 psmExchangeRate)
external
override
onlyOwner
notDelegated
returns (address ct, address ds)
{
Pair memory asset = Pair(pa, ra);
// prevent deploying a swap asset of a non existent pair, logically won't ever happen
// just to be safe
if (lvs[asset.toId()] == address(0)) {
revert NotExist(ra, pa);
}
string memory pairname = string(abi.encodePacked(Asset(ra).name(), "-", Asset(pa).name()));
ct = address(new Asset(CT_PREFIX, pairname, owner, expiry, psmExchangeRate));
ds = address(new Asset(DS_PREFIX, pairname, owner, expiry, psmExchangeRate));
swapAssets[Pair(pa, ra).toId()].push(Pair(ct, ds));
deployed[ct] = true;
deployed[ds] = true;
emit AssetDeployed(ra, ct, ds);
}As can be seen from the above code snippets when the issueNewDs() function reverts, the CT token won't exist and the pair contract won't exist. Secondly, you can't simply donate RA assets to the pair, you have to mint LP tokens, and it is a bit hard to mint LP tokens when one of the tokens of the pair doesn't exist and its supply can't be more than 0. In the mint() function: liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);Multiplication by 0 will always be 0. function createPair(address tokenA, address tokenB) external returns (address pair) {
require(tokenA != tokenB, 'UniswapV2: IDENTICAL_ADDRESSES');
(address token0, address token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
require(token0 != address(0), 'UniswapV2: ZERO_ADDRESS');
require(getPair[token0][token1] == address(0), 'UniswapV2: PAIR_EXISTS'); // single check is sufficient
bytes memory bytecode = type(UniswapV2Pair).creationCode;
bytes32 salt = keccak256(abi.encodePacked(token0, token1));
assembly {
pair := create2(0, add(bytecode, 32), mload(bytecode), salt)
}
IUniswapV2Pair(pair).initialize(token0, token1, dsFlashSwapRouter);
getPair[token0][token1] = pair;
getPair[token1][token0] = pair; // populate mapping in the reverse direction
allPairs.push(pair);
emit PairCreated(token0, token1, pair, allPairs.length);
}As can be seen from the code snippet above getPair will be set to the address of the newly deployed pair, and if the function is called again with the same tokens it will revert: require(getPair[token0][token1] == address(0), 'UniswapV2: PAIR_EXISTS'); // single check is sufficientThe issueNewDs() function attempts to call the createPair() function address ammPair = getAmmFactory().createPair(ra, ct);We can even speculate that the protocol won't be used by anyone an thus there won't be any vulnerabilities in it, but this is not the point of the audit and it doesn't change the fact that the protocol doesn't work as expected, and one of its most critical functions reverts in certain cases. |
It is possible, ra can be donated before the pair is created. So when it reaches the mint() function in the pair, it would be as if more ra was deposited, leading to liquidity above |
A bit difficult, I would say even impossible when you have this in the mint function liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY); |
|
Admin calculates the pair address and sends a dust ra to the pair. The pair now has a dust ra balance before being created. |
|
The CT token doesn't exist he can't donate it. The CT token is created in the issueNewDS function ct = address(new Asset(CT_PREFIX, pairname, owner, expiry, psmExchangeRate));
ds = address(new Asset(DS_PREFIX, pairname, owner, expiry, psmExchangeRate)); |
|
Only ra needs to be donated, ct will always be non null due to this check. |
|
I don't think that "donating tokens to the contract" to prevent low liquidity is the correct reason to invalidate the issue. Otherwise, any issue about low liquidity could be mitigated by users just donating tokens into the contract to get their stuck tokens. Hence, my decision remains, reject the escalation and leave the issue as it is. |
|
@WangSecurity it's not to prevent, but to fix. So the DoS is 1 block. |
|
In order to donate the new CT token you first have to have it - you have to mint it. The contract for the CT token doesn't exist until the ModuleCore::issueNewDs() function is called. The function reverts. You can't mint ERC20 tokens from a contract that doesn't exist. After that even if you donate all the RA tokens in existence it won't matter. liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);this will always try to subtract minimum liquidity from 0. |
|
No need to use ct tokens, the explanation of how the fix works is here. |
|
@0xsimao for the last time you can't donate a token that doesn't exist, the calculation is clearly amount0 * amount1 - something. The CT token simply doesn't exist, it is supposedly deployed in the issueNewDS() function, however that function will revert thus the token won't exist. if amount0 is the CT token 0 * type(uint128).max is 0. What you are suggesting is simply not possible. |
|
Some dust ct exists and is deposited to the pool, which is enough to donate ra to the pool such that the calculation exceeds |
|
How does it exist when there is no contract deployed for that token |
|
|
That is not correct issewNedDs() deploys new ERC20 contracts for CT and DS (address ct, address ds) = IAssetFactory(SWAP_ASSET_FACTORY).deploySwapAssets(
ra, state.info.pair0, address(this), expiry, exchangeRates
); function deploySwapAssets(address ra, address pa, address owner, uint256 expiry, uint256 psmExchangeRate)
external
override
onlyOwner
notDelegated
returns (address ct, address ds)
{
Pair memory asset = Pair(pa, ra);
// prevent deploying a swap asset of a non existent pair, logically won't ever happen
// just to be safe
if (lvs[asset.toId()] == address(0)) {
revert NotExist(ra, pa);
}
string memory pairname = string(abi.encodePacked(Asset(ra).name(), "-", Asset(pa).name()));
ct = address(new Asset(CT_PREFIX, pairname, owner, expiry, psmExchangeRate));
ds = address(new Asset(DS_PREFIX, pairname, owner, expiry, psmExchangeRate));
swapAssets[Pair(pa, ra).toId()].push(Pair(ct, ds));
deployed[ct] = true;
deployed[ds] = true;
emit AssetDeployed(ra, ct, ds);
}
Then it creates a pair for the newly created CT token with RA(RA is a token that is not deployed by the Cork team, for example ETH) address ammPair = getAmmFactory().createPair(ra, ct);Given the fact that the issueNewDs() function will revert so will the deployment of the CT token. If the admin is not Gandalf it is not possible to donate a token that doesn't exist. |
|
It is possible to donate any token to any address without it having code - and only ra is needed. The ct token is deployed before |
|
The rounding error occurs here (MathHelper) // with 1e18 precision
ratePerLv = ((totalAmount * 1e18) / totalLvIssued);
// attribute all to AMM if no lv issued or withdrawn
if (totalLvIssued == 0 || totalLvWithdrawn == 0) {
return (0, totalAmount, ratePerLv);
}
attributedWithdrawal = (ratePerLv * totalLvWithdrawn) / 1e18;
attributedAmm = totalAmount - attributedWithdrawal;
|
|
The rounding error occurs here (MathHelper) // with 1e18 precision
ratePerLv = ((totalAmount * 1e18) / totalLvIssued);
// attribute all to AMM if no lv issued or withdrawn
if (totalLvIssued == 0 || totalLvWithdrawn == 0) {
return (0, totalAmount, ratePerLv);
}
attributedWithdrawal = (ratePerLv * totalLvWithdrawn) / 1e18;
attributedAmm = totalAmount - attributedWithdrawal;
|
|
Hey @WangSecurity there are several internal calls that the protocol makes, you are mostly correct in your statement. It would be very hard to track everything and explain it line by line. In your first example when we are issuing CT & DS tokens for a second or third time for example there is a call to _liquidatedLp() - this function has to liquidate all the LP that the Cork protocol owns. The Ra and CT amounts returned from that liquidation are the main factors that determine what the rationedToAmm() function will return. Now if rationedToAmm() returns RA > 0, and CT = 0, we have a problem, no matter how much RA is donated the mint() in the uniV2 pair will always revert. The admin can manipulate the amount of tokens in the RA:CT1 pair(this is the already deployed uniV2 pair, which we are trying to liquidate LP from). The purpose of the admin manipulating the amount of tokens in the RA:CT1 pair is so such amounts are returned from liquidation that either makes CT > 0, or RA = 0 and CT = 0, the admin can do that via donating tokens directly to the uniV2 paid, swapping, or if he had previously minted LP tokens directly from the uniV2 pair he can burn a portion of them, note that LV and LP tokens are different. However, a malicious actor can perform the same steps in order to manipulate the amounts of RA and CT that are returned when the Cork protocol liquidates the LP it owns and so we go back to the scenario where RA > 0 and CT = 0. The only way the admin can guarantee he is going to successfully mitigate this issue is if he first manipulates the price of the RA:CT1 pair, and then immediately calls issueNewDs(). Thus the admin will have to make sure nobody manipulates the rate again before he can call issueNewDs() - either make sure his transaction is the first in the block, or submit it trough some private rpcs. I belive we have established those actions are not something the admin is expected to perform. |
|
Someone stopping the admin from fixing this issue when ra > 0 and ct = 0 by frontrunning them and tweaking the price from the previous pool so that rounding errors always occur is 1 block DoS. |
|
It is not a 1 block DOS, because the issue already exists, only the admin trying to fix the issue is dosed. It is not someone donating 1 WEI to stop someone from withdrawing for example, in this scenario the issue occurs because of the donation thus it can be considered 1 block DOS. Here the issue already exists and someone is trying to fix it by executing a series of actions, if those actions are dosed or prevented this is regarding the fix, not the issue itself. |
|
So, to mitigate the issue if it comes up in reality, the admin can make a swap on RA:CT1 pair so both are non-0 and then call issueNewDs, correct? But, if it's not in the same transaction, then:
But we can continue here, and the admin again swaps RA: CT1 and calls issueNewDs, and there will be no problem unless someone makes another swap, i.e. repeat the "attack". Correct? Without this attack repeated, i.e. without users swapping after the admin has swapped, would there be no issue? |
|
Yes, or the admin can frontrun them. But given the fact that the code doesn't work as expected and we are presenting a series of admin actions as a potential fix, we can only say that the admin is guaranteed to fix the issue only if he frontruns - thus he has to perform actions which are not expected from him as you have previously stated + I believe we shouldn't even be discussing the possibility of this admin fix because the code that was presented clearly doesn't work, which satisfies the requirements for a medium. |
Yep, that is right, attackers need to keep frontrunning the admin, that is, repeating the attack. Also, as said previously, the admin can use a private rpc or a multicall to batch transactions. This is a fix, not a prevention, the admin can no anything. |
|
I agree that the code doesn't work as expected here and leads to locked funds. But, if this comes up in reality, the admin could resolve the issue, and the funds would be unlocked in fairly less than seven days. Yes, someone could front-run the admin calling
So, considering DOS caused by the attacker's swap to be only one block, the admin would be able to call issueNewDs, and all the funds would be unlocked.
Hence, this is a low-severity finding though a very good catch. Planning to accept the escalation and invalidate the issue tomorrow after 10 AM UTC. |
|
@WangSecurity I strongly disagree with the way the rules are interpreted and applied to this issue. First of all in regard to a similar admin issue in #214 you said:
Admin is not supposed to be fixing problems in the code with donations and manipulating the price of a uniV2 pair. When it comes to the rule about 1 block DOS, as I have clarified previously the issue exists not because a malicious actor causes it, but because the code doesn't work as expected. The fix by the admin is impractical - because he has to either fronturn - which we established earlier is not an expected behavior, or he has to hope that nobody changes the rate. This clearly becomes a very impractical and certainly not an expected action by the admin. The DOS rules clearly talks about issues not admin actions trying to fix an already existing issue: The ISSUE impacts the availability of time-sensitive functions. Without the admin actions the funds will be locked forever and the whole protocol will be bricked. I believe two rules are applied in a way that is detrimental to the impact presented in this report, I don't believe we can expect an admin to fix issues by donations and manipulating uniV2 prices and if this fix doesn't work in 100% of the cases we can say that this fix is sufficient. Clearly here not expected admin actions are being dosed, the potential attacker is not dosing any functionality of the protocol directly. We can not expect the admin to fix something by executing a series of actions, and then apply the 1 block DOS rule on those actions. Applying the 1 block DOS rule in favour of the admin actions is equivalent to saying the admin will frontrun other users or submit the transaction via a private rpc. We can only consider that the admin can sufficiently fix a preexisting issue if his actions can't be denied. |
The problem in #214 is that the admin would have to create dummy CTs, which may take hundreds of pairs to be created. This is why I don't apply the admin rules there. Here, it's only making a swap on the RA:CT pair and/or dust donation, which I don't see as impractical.
To clarify, I don't apply the 1-block DOS rule to the entire issue, but to a specific part:
Here, the attacker would have to repeat the swap each time to persist this DOS. But one iteration of his swap leads to only one-block DOS. So, I don't apply the 1-block DOS rule to the entire issue but to the part where the attacker swaps to DOS
However, the admin's actions that can resolve the issue if it happens are taken into account when determining the severity/validity.
That's a fair concern, but due to the fact that the donation can be smaller than 1e4 or even no donation and only a swap, this is taken into account here.
Hope my elaboration above cleared the confusion about 1-block DOS. Excuse me for not explaining it clearly in the beginning. The decision remains: accept the escalation and invalidate the issue. Planning to apply this decision in a couple of hours. |
|
@WangSecurity you are applying the 1 DOS block rule exactly to the part where you shouldn't. There is no place else where you can apply this rule.
If you apply the 1 block DOS rule to this exact part is the exact same thing as saying the admin will frontrun or run the transaction trough a private rpc. It makes absolutely no difference. Why would someone need to frontrun something if we expect he will get the result he wants? Fontruning in this case will be absolutely the same as saying the admin actions can't be prevented via another swap. We established that admin is not supposed to frontrun or execute transactions via private rpcs, however when you apply the 1 block DOS rule to this you are basically saying by default admin frontruns everybody else, this is not correct. |
Applying the rule and saying the admin would front-run anyone is not the same, and this situation perfectly fits the situation. The attacker's swap would DOS
Here is the attacker front-runs the admin to fail his transaction, but they need to do it every time to keep the DOS of |
|
@WangSecurity your answer lacks any logic, I understand it is a bit hard to see how frontrunning and quoting the 1 block DOS are different when you lack experience and a deep understanding of the code base. I don't know what words to use so you stop mindlessly quoting the same incorrect thing. Please explain to me logically what the difference is between the admin frontrunning the potential swap of another user, and saying another user swapping is a dos for 1 block you are clearly stating that another user is not supposed to interact with the swap, and this is so wrong I can start to describe it. Do you understand how fronturning works in real life, and why it is used? It is not normal for you to refuse to acknowledge this. The arguments you have provided in the discussion in #214 are mutually exclusive. Given the fact you can’t provide a logical argument as to why dos for 1 block is different from frontruning in this scenario (mindlessly quoting the rule from the rule book, is not a logical argument), either make a correct logical argument or validate the issue. |
|
To understand your arguments, do you think this is valid because the attacker would continue to make swaps after the admin to keep the DOS? And the only way to stop this is the admin front-running the attacker's swap?
|
|
Yes, this is the only way this issue can be mitigated at 100%, as I have stated previously the issue is not caused by an attacker dossing a function of the protocol for 1 block, the admin actions trying to fix an issue that originated from the code base can be denied by another actor. Otherwise, the admin can only hope for the best and this does not guarantee a fix of this issue via admin actions. If the admin actions can't guarantee that the issue is fixed, why are we even considering this as a potential fix? It either works in 100% of the cases or the issue continues to exist. |
|
It is not reasonable to expect the admin will fail to fix this within 7 days. Also, a multicall can be used, which is bulletproof. |
|
Expecting admin to fix issues originating from a vulnerable code is not reasonable for me as well, but apparently, we are considering it. Not reasonable is not the same as it can't happen. Multicall is not bulletproof by itself because it can be frontrun and the swap transaction can be nullified if the frontrunner swaps with arguments that counter the admin swap. Nevertheless, this will be my last comment, I believe we wasted more time on this contest than we should have. I hope the correct decision is made, and the same standards are applied to each issue. Have a nice evening/day, everybody! |
|
We need to consider if an issue can be fixed in less than 7 days to figure low or medium severity. With the multicall any logic can be placed, so the price can always be tweaked correctly. |
|
I'll try to re-iterate again explaining why 1-block DOS is applicable. Here's the issue and how it could be resolved if it comes up:
But, an attacker can swap as well and the path would become the following:
For the I see that the initial issue wasn't caused by an attacker. But the initial issue can be resolved easily by making just one swap (or two). That's why it should be low severity, because it's causes lock of funds, but the funds can be unlocked in less than a week fairly. Hence, my decision to invalidate remains. Will apply it in a couple of hours. If the dispute continues to use the same arguments of "the admin has to use private RPC/multicalls/front-running to resolve the issue, "the fact that the admin can resolve it shouldn't be taken into account", "the rule is interpreted incorrectly", "you don't understand the code", or other arguments that were already answered. The decision will be applied without a time window to dispute. |
|
This discussion is pointless, invalidate the issue and release the results. Let's enjoy the can of warms that was open in this contest. |
sherlock-admin2
added
Non-Reward
|
Result:
|
sherlock-admin3
added
the
Escalation Resolved
|
Escalations have been resolved successfully! Escalation status:
|
dimulski
Medium
If all LV tokens are requested for redeem, issuing new CT and DS tokens will revert
Summary
The Corc protocol creates a pair for RA:PA tokens, and then can issue CT and DS tokens for that pair, which can be used for different purposes such as hedging against PA tokens deepening from the RA token. The CT and DS tokens have an expiration. Once they have expired the protocol admins can issue new CT and DS tokens via the ModuleCore::issueNewDs() function. However in some cases that won't be possible. The ModuleCore::issueNewDs() function calls a lot of functions that have to do with deploying asset contracts and setting up parameters, but the problem arises in the VaultLib::onNewIssuance() function, which calls the VaultLib::__provideAmmLiquidityFromPool() function:
There are several overengineered calls that first separate the liquidity for the previously expired CT and DS tokens, based on how much LV tokens users minted, and how much of those LV tokens were requested for withdrawal. If all of the users that minted LV tokens request to withdraw them, the internal call to VaultPoolLib::rationedToAmm() function will return 0. And when the contract tries to deposit 0 RA and 0 CT tokens to a newly created UniV2 Pair, the contract will revert due to underflow.
All users requesting to redeem their LV in the same time frame is fairly possible scenario. Keep in mind that there is one LV token for a RA:PA pair, and several CT and DS tokens can be issued for a pair of RA:PA tokens. So it just may happens that at the 3rd issuing of CT and DS tokens, all users have requested to redeem their LV tokens.
Root Cause
In the VaultLib::__provideAmmLiquidityFromPool() function there is no check whether there is liquidity that can be provided to the newly created UniV2 pair.
Internal pre-conditions
External pre-conditions
No response
Attack Path
No response
Impact
The ModuleCore::issueNewDs() function is most critical function for the protocol, without it new DS and CT tokens can't be issued and the protocol becomes obsolete, simply redeploying the protocol won't fix this issue, as there can be other pairs for RA:PA tokens, and there can be tokens that users are still to reclaim, keep in mind that once CT and DS tokens have expired RA tokens that were deposited by users can still be withdrawn from users by calling the Psm::redeemWithCT() function and depositing CT tokens, that have already expired. Thus the medium severity.
PoC
Gist
After following the steps in the above mentioned gist add the following test to the
AuditorTests.t.solfile:To run the test use:
forge test -vvv --mt test_IssuingNewDsWillRevertWhenNoLiquidityCanBeProvidedMitigation
In the VaultLib::__provideAmmLiquidityFromPool() function first check whether there is liquidity that can be provided.
The text was updated successfully, but these errors were encountered: