sherlock-admin2
commented
6 months ago 1 comment(s) were left on this issue during the judging contest.
takarez commented:
valid: minRewaiWeight should be mitigated; medium(7)
Closed sherlock-admin closed 6 months ago
sherlock-admin2
commented
6 months ago 1 comment(s) were left on this issue during the judging contest.
takarez commented:
valid: minRewaiWeight should be mitigated; medium(7)
nevillehuang
commented
6 months ago Invalid, agree with sponsors comment:
- Both minReward and maxReward are set by privileged role, wherein they are order of magnitudes different from each other.
- minReward is from minmargin which is stored per account → impossible to loop over all accounts everytime a new maxReward is set
Kalyan-Singh
commented
6 months ago Escalate
I respectfully disagree with the judge and sponsors comments, which state
- Both minReward and maxReward are set by privileged role, wherein they are order of magnitudes different from each other.
- minReward is from minmargin which is stored per account → impossible to loop over all accounts everytime a new maxReward is set
I will make 2 arguments
There is no need to loop over the accounts the minimumMargin value is passed to LendingPool dynamically when its liquidation is started as can be verified here & here this dynamic value is then passed to _calculateRewards, a very simple fix can be
function _calculateRewards(uint256 debt, uint256 minimumMargin_)
internal
view
returns (uint256 initiationReward, uint256 terminationReward, uint256 liquidationPenalty)
{
uint256 maxReward_ = maxReward;
// The minimum reward, for both the initiation- and terminationReward, is defined as a fixed percentage of the minimumMargin.
uint256 minReward = minimumMargin_.mulDivUp(minRewardWeight, ONE_4);
// Initiation reward must be between minReward and maxReward.
initiationReward = debt.mulDivDown(initiationWeight, ONE_4);
initiationReward = initiationReward > minReward ? initiationReward : minReward;
initiationReward = initiationReward > maxReward_ ? maxReward_ : initiationReward;
// Termination reward must be between minReward and maxReward.
terminationReward = debt.mulDivDown(terminationWeight, ONE_4);
terminationReward = terminationReward > minReward ? terminationReward : minReward;
terminationReward = terminationReward > maxReward_ ? maxReward_ : terminationReward;
+ if(initiationReward + terminationReward > maxReward_){
+ initiationReward = maxReward_.mulDivDown(initiationWeight,ONE_4);
+ terminationReward = maxReward_.mulDivDown(terminationWeight,ONE_4);
+ }
liquidationPenalty = debt.mulDivUp(penaltyWeight, ONE_4);
}All the liquidationParameters and minimumMargin are upgradeble by the admins, that can severely amplify the issue. Lets say currently the LendingPool has 200 $ minimumMargin and maxRewards = 200 /2 =100 $ , in this case the initiation + terminationReward can never exceed minimumMargin. But now the protocol decides to upgrade to a minimumMargin of 300 $ & maxRewards is accordingly capped at 150 $. But this change is problematic as minimumMargin from 'AccountV1.sol' is used (old cached value) but maxReward's new value is used in _calculateRewards, which is a state mismatch . So even if the admins do everything right liquidating some old accounts which have cached in old values of minimumMargin which is not enough to pay the incentives and loss to LPs is caused.
Lastly I would like to point out the following sherlock rule,
I think I have sufficiently shown the scenarios where constraints in place are not enough to prevent loss to LPs even if the inputs are from admins.
sherlock-admin2
commented
6 months ago Escalate
I respectfully disagree with the judge and sponsors comments, which state
- Both minReward and maxReward are set by privileged role, wherein they are order of magnitudes different from each other.
- minReward is from minmargin which is stored per account → impossible to loop over all accounts everytime a new maxReward is set
I will make 2 arguments
There is no need to loop over the accounts the minimumMargin value is passed to LendingPool dynamically when its liquidation is started as can be verified here & here this dynamic value is then passed to _calculateRewards, a very simple fix can be
function _calculateRewards(uint256 debt, uint256 minimumMargin_) internal view returns (uint256 initiationReward, uint256 terminationReward, uint256 liquidationPenalty) { uint256 maxReward_ = maxReward; // The minimum reward, for both the initiation- and terminationReward, is defined as a fixed percentage of the minimumMargin. uint256 minReward = minimumMargin_.mulDivUp(minRewardWeight, ONE_4); // Initiation reward must be between minReward and maxReward. initiationReward = debt.mulDivDown(initiationWeight, ONE_4); initiationReward = initiationReward > minReward ? initiationReward : minReward; initiationReward = initiationReward > maxReward_ ? maxReward_ : initiationReward; // Termination reward must be between minReward and maxReward. terminationReward = debt.mulDivDown(terminationWeight, ONE_4); terminationReward = terminationReward > minReward ? terminationReward : minReward; terminationReward = terminationReward > maxReward_ ? maxReward_ : terminationReward; + if(initiationReward + terminationReward > maxReward_){ + initiationReward = maxReward_.mulDivDown(initiationWeight,ONE_4); + terminationReward = maxReward_.mulDivDown(terminationWeight,ONE_4); + } liquidationPenalty = debt.mulDivUp(penaltyWeight, ONE_4); }All the liquidationParameters and minimumMargin are upgradeble by the admins, that can severely amplify the issue. Lets say currently the LendingPool has 200 $ minimumMargin and maxRewards = 200 /2 =100 $ , in this case the initiation + terminationReward can never exceed minimumMargin. But now the protocol decides to upgrade to a minimumMargin of 300 $ & maxRewards is accordingly capped at 150 $. But this change is problematic as minimumMargin from 'AccountV1.sol' is used (old cached value) but maxReward's new value is used in _calculateRewards, which is a state mismatch . So even if the admins do everything right liquidating some old accounts which have cached in old values of minimumMargin which is not enough to pay the incentives and loss to LPs is caused.
Lastly I would like to point out the following sherlock rule,
I think I have sufficiently shown the scenarios where constraints in place are not enough to prevent loss to LPs even if the inputs are from admins.
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.
Kalyan-Singh
commented
6 months ago Now that i think more about it caching minimumMargin in 'AccountV1.sol' leads to a few more severe issues, like when the riskParameters and minimumMargin are updated, except minimumMargin all other values are loaded dynamically and reflect the latest state. But the cached minimumMargin is used everywhere, even in the getUsedMargin() function. Consider the example, Alice opens a account, currently minimumMargin in 'LendingPool.sol' = 200 $, which gets cached in her account. She deposits 400 $ and opens a position of 100 $. So getUsedMargin() for alice = 300 $ collateralValue = 400 $
Now after some time admins/riskManager update the risk parameters and minimumMargin is also updated to 300 $ . Now bob opens a Account his minimumMargin will be cached at 300 $, he deposits 410 $ collateral & opens position of 100 $ . So getUsedMargin() for bob = 400 $ collateralValue = 410 $
Assuming both of them gave the same collateral at same price. But now the collateral's value decreases by 11 $, at this point bob's position is unhealthy but alice's position is still healthy. So bob gets liquidated. Which is unfair both to bob and the LPs.
Here's a coded poc-
/**
* Created by Pragma Labs
* SPDX-License-Identifier: BUSL-1.1
*/
pragma solidity 0.8.22;
import "./_LendingPool.fuzz.t.sol";
import "../../../lib/accounts-v2/test/fuzz/Fuzz.t.sol";
import "../Fuzz.t.sol";
import "forge-std/console.sol";
import { ActionData } from "../../../lib/accounts-v2/src/interfaces/IActionBase.sol";
import { IPermit2 } from "../../../lib/accounts-v2/src/interfaces/IPermit2.sol";
// import "../../../src/Liquidator.sol";
import { AssetValuationLib } from "../../../lib/accounts-v2/src/libraries/AssetValuationLib.sol";
contract LendingPoolUnderstanding is LendingPool_Fuzz_Test {
address alice;
address bob;
address charlie;
AccountV1 aliceAccount;
AccountV1 aliceAccount2;
AccountV1 bobAccount;
function setUp() public override {
super.setUp();
alice = makeAddr("alice");
bob = makeAddr("bob");
charlie = makeAddr("charlie");
vm.startPrank(users.liquidityProvider);
mockERC20.stable1.burn(mockERC20.stable1.balanceOf(users.liquidityProvider));
vm.stopPrank();
vm.label(alice,"Alice");
}
function depositInAcc(ERC20Mock mockToken, AccountV1 account, uint amt) public {
address owner = account.owner();
vm.startPrank(owner);
mockToken.mint(owner,amt);
mockToken.approve(address(account),amt);
address[] memory assets = new address[](1);
uint[] memory assetIds = new uint[](1);
uint [] memory assetAmts = new uint[](1);
assetAmts[0] = amt;
assets[0] = address(mockToken);
account.deposit(assets,assetIds,assetAmts);
vm.stopPrank();
}
function testCachingProblem() public {
uint256 minimumMargin = 200 * 1e6;
uint depositAmt = 400 * 1e6; //depositAmt of mockStable2
uint openPosition = 100 * 1e6;
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(minimumMargin));
vm.stopPrank();
// setting riskParams
vm.startPrank(users.riskManager);
registryExtension.setRiskParametersOfPrimaryAsset(
address(pool),
address(mockERC20.stable2),
0,
type(uint112).max,
uint16(AssetValuationLib.ONE_4),
uint16(AssetValuationLib.ONE_4)
);
vm.stopPrank();
// checking minimum margin set
(,,,uint minimumMargin_) = pool.openMarginAccount(1);
assertEq(minimumMargin,minimumMargin_);
// alice creates Account
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
// depositing in jr tranche
mockERC20.stable1.mint(address(this),50000 * 1e6);
mockERC20.stable1.approve(address(pool), 50000 * 1e6);
jrTranche.deposit(50000 * 1e6, bob);
depositInAcc(mockERC20.stable2, aliceAccount, depositAmt);
// alice takes loan
vm.startPrank(alice);
pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0));
vm.stopPrank();
console.log("----------------After Loan----------------");
emit log_named_decimal_uint("openPosAlice", pool.getOpenPosition(address(aliceAccount)), 6);
emit log_named_decimal_uint("getUsedMarginAlice",aliceAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueAlice",aliceAccount.getCollateralValue(),6);
console.log();
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(300 * 1e6));
vm.stopPrank();
vm.startPrank(bob);
bobAccount = AccountV1(factory.createAccount(5678, 0, address(pool)));
vm.stopPrank();
depositInAcc(mockERC20.stable2, bobAccount, 410 * 1e6);
// alice takes loan
vm.startPrank(bob);
pool.borrow(openPosition, address(bobAccount), bob, bytes3(0));
vm.stopPrank();
console.log("----------------After Bob Loan----------------");
emit log_named_decimal_uint("openPosBob", pool.getOpenPosition(address(bobAccount)), 6);
emit log_named_decimal_uint("getUsedMarginBob",bobAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueBob",bobAccount.getCollateralValue(),6);
console.log();
transmitOracle(mockOracles.stable2ToUsd,0.973 ether);
assertEq(bobAccount.isAccountLiquidatable() , true);
assertEq(bobAccount.isAccountUnhealthy(),true);
assertEq(aliceAccount.isAccountLiquidatable(), false);
assertEq(aliceAccount.isAccountUnhealthy() , false);
console.log("----------------At End----------------");
emit log_named_decimal_uint("openPosAlice", pool.getOpenPosition(address(aliceAccount)), 6);
emit log_named_decimal_uint("getUsedMarginAlice",aliceAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueAlice",aliceAccount.getCollateralValue(),6);
emit log_named_decimal_uint("openPosBob", pool.getOpenPosition(address(bobAccount)), 6);
emit log_named_decimal_uint("getUsedMarginBob",bobAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueBob",bobAccount.getCollateralValue(),6);
}
}add it to test/fuzz/LendingPool/ dir and run using
forge test --match-test testCachingProblem -vvThe fix for this is simple-
make minimumMargin a public variable in LendingPool.sol, add the following function to ICreditor.sol
function minimumMargin() external view returns(uint96);and call this function every time 'AccountV1.sol' needs to know minimumMargin value for e.g getUsedMargin() will look like
function getUsedMargin() public view returns (uint256 usedMargin) {
// Cache creditor
address creditor_ = creditor;
if (creditor_ == address(0)) return 0;
// getOpenPosition() is a view function, cannot modify state.
- usedMargin = ICreditor(creditor_).getOpenPosition(address(this)) + minimumMargin;
+ usedMargin = ICreditor(creditor_).getOpenPosition(address(this)) + ICreditor(creditor_).minimumMargin();
}This is not a dup of #1 the above mentioned issue talks about a different scope(i.e minUsdValue param stored in 'Registry.sol', #162 does not have anything to do with that param), a different vector(i.e account gets upgraded and registry changes but 162 does not require a account upgrade or registry change) and totally different impact(i.e user being liquidated if he incorrectly upgrades without his due dilligence) . It is not related to this issue.
Thomas-Smets
commented
6 months ago As we mentioned in the comments why we set a minimum and maximum liquidation penalty, both serve a very different purpose and are multiple orders of magnitude different.
minReward is derived from minimumMargin and prevents that their are no dust positions that are unprofitable for liquidators to liquidate since their only cost are gas costs, this in the order of a few $ on L2s.maxReward is their since the cost for initiating and terminating is fixed and does not scale wit the position size. While we do give out bigger rewards for bigger positions (since they are more risky) we cap it at some point for very big positions.
Hence 'maxReward' is in the order of a 1000s of $Next, note that both minimumMargin and maxReward are set by privileged roles.
They just need to ensure that a new minimumMargin is smaller than any maxReward ever set for that Creditor, and that any new maxReward is bigger than any minimumMargin ever set.
But as explained above there is no need to explicitly check it, since both are orders of magnitude different.
Also, since it is a privileged role that sets it, it is not considered according to the Sherlock rules:
https://github.com/sherlock-protocol/sherlock-v2-docs/blob/60b2cb8caa0e4e43f71694f70bb08bf06326e085/audits/judging/judging/README.md?plain=1#L80
Lastly, take the worst case example, where the minReward is indeed bigger than maxReward.
In that case the reward would be equal to maxReward.
This would still not lead to direct losses for LPs:
minReward that was set to high, we were just over conservative and liquidation will be handled fine.maxReward that was set to low, we have a dust position by definition, for which it is hard to start and end liquidations, but not impossible, Any stakeholder can still trigger the start and end.There is no need to loop over the accounts the minimumMargin value is passed to LendingPool dynamically when its liquidation is started as can be verified here & here this dynamic value is then passed to _calculateRewards
The value initially comes from the Account, where it is stored, see: https://github.com/arcadia-finance/accounts-v2/blob/9b24083cb832a41fce609a94c9146e03a77330b4/src/accounts/AccountV1.sol#L533 So yes we should loop over all Accounts.
Kalyan-Singh
commented
6 months ago As we mentioned in the comments why we set a minimum and maximum liquidation penalty, both serve a very different purpose and are multiple orders of magnitude different.
* The `minReward` is derived from `minimumMargin` and prevents that their are no dust positions that are unprofitable for liquidators to liquidate since their only cost are gas costs, this in the order of a few $ on L2s. * The `maxReward` is their since the cost for initiating and terminating is fixed and does not scale wit the position size. While we do give out bigger rewards for bigger positions (since they are more risky) we cap it at some point for very big positions. Hence 'maxReward' is in the order of a 1000s of $Next, note that both
minimumMarginandmaxRewardare set by privileged roles. They just need to ensure that a newminimumMarginis smaller than anymaxRewardever set for that Creditor, and that any newmaxRewardis bigger than anyminimumMarginever set. But as explained above there is no need to explicitly check it, since both are orders of magnitude different. Also, since it is a privileged role that sets it, it is not considered according to the Sherlock rules: https://github.com/sherlock-protocol/sherlock-v2-docs/blob/60b2cb8caa0e4e43f71694f70bb08bf06326e085/audits/judging/judging/README.md?plain=1#L80Lastly, take the worst case example, where the
minRewardis indeed bigger thanmaxReward. In that case the reward would be equal tomaxReward. This would still not lead to direct losses for LPs:* If it was `minReward` that was set to high, we were just over conservative and liquidation will be handled fine. * If it was `maxReward` that was set to low, we have a dust position by definition, for which it is hard to start and end liquidations, but not impossible, Any stakeholder can still trigger the start and end.There is no need to loop over the accounts the minimumMargin value is passed to LendingPool dynamically when its liquidation is started as can be verified here & here this dynamic value is then passed to _calculateRewards
The value initially comes from the Account, where it is stored, see: https://github.com/arcadia-finance/accounts-v2/blob/9b24083cb832a41fce609a94c9146e03a77330b4/src/accounts/AccountV1.sol#L533 So yes we should loop over all Accounts.
"Lastly, take the worst case example, where the minReward is indeed bigger than maxReward."
There have been some misunderstandings i think, the bug is not about minRewards being greater than maxReward
setLiquidationParameters quotes "Sum of the initiationReward and terminationReward cannot exceed minimumMargin of the Account. hence minRewardWeight is capped to 50%."
This quote shows that initiationRewards + terminationReward should be < = minimumMargin. -- point (1)
So now lets take a example within the limit told by the sponsor in above comments i.e minReward is in order order $ , maxReward is in 1000s$ and that minimumMargin is smaller than maxRewards.
this line in LendingPool states "minimumMargin is the fixed gas cost independent of openDebt", -- point(2) since the sponsors do not seem statisfied with my 200 $ value for minimumMargin in above POCs lets take it to be 50 $ which should be enough on L2s. Matter of fact they can take it to be even less and try running the poc with that.
maxRewards = 1000 $ minimumMargin = 50 $
uint256 minReward = minimumMargin_.mulDivUp(minRewardWeight, ONE4);[link](uint256 minReward = minimumMargin.mulDivUp(minRewardWeight, ONE_4);)
so lets take minReward = 5 $ so consecuently minRewardWeight = 10% = 1000 since 1000 / ONE_4 = 10% = 5 $ initiationWeight = 100 -> 1% used everywhere in tests and is fair enough terminationWeight = 50 -> 0.5% used everywhere in tests and is fair enough
Now lets say Alice has a openPosition of 24950 usdc , her accounts becomes unhealthy , charlie starts liquidations
so, calculateRewards will give the following outputs
initiationReward = debt.mulDivDown(initiationWeight , ONE_4) = 24950 usdc * 1% = 249.5 $ which is greater than minReward but less than maxReward so it will remain as it is as can be seen here terminationReward = debt.mulDivDown(terminationWeight, ONE_4) = 24950 usdc * 0.5% = 124.75 $ , which is also within constraints
So now initiationReward + terminationReward = 374.25 $ which breaks point(1)
But more importantly, since the account's minimumMargin is only offsetting 50 $ , the remaining 324.25 $ is paid from the pocket of LPs, which is a significant loss of yield
To verify you can switch testExtraLiqRewards() in the poc provided in original submission to this
function testExtraLiqRewards() public {
uint256 minimumMargin = 50 * 1e6;
uint depositAmt = 25000 * 1e6; //depositAmt of mockStable2
uint openPosition = 24950 * 1e6;
// depositing amount into charlie
// mockERC20.stable1.mint(charlie,200 * 1e6) ;
assertEq(mockERC20.stable1.balanceOf(charlie),0);
vm.prank(alice);
mockERC20.stable1.approve(address(pool),UINT256_MAX);
{
vm.startPrank(pool.owner());
(uint16 initiationW , uint16 penaltyW , uint16 terminationW , uint16 minRewardWeight , uint80 maxReward)=pool.getLiquidationParameters();
// setting max reward to 1000$ and minReward weight to 2%
pool.setLiquidationParameters(initiationW, penaltyW, terminationW, 1000, 1000 * 1e6);
emit log_named_uint("penaltyW", penaltyW);
emit log_named_uint("initiationW", initiationW);
emit log_named_uint("terminationW", terminationW);
vm.stopPrank();
vm.startPrank(users.riskManager);
registryExtension.setRiskParametersOfPrimaryAsset(
address(pool),
address(mockERC20.stable2),
0,
type(uint112).max,
uint16(AssetValuationLib.ONE_4),
uint16(AssetValuationLib.ONE_4)
);
vm.stopPrank();
}
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(minimumMargin));
vm.stopPrank();
// checking minimum margin set
(,,,uint minimumMargin_) = pool.openMarginAccount(1);
assertEq(minimumMargin,minimumMargin_);
// alice creates Account
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
// depositing in jr tranche
mockERC20.stable1.mint(address(this),50000 * 1e6);
mockERC20.stable1.approve(address(pool), 50000 * 1e6);
jrTranche.deposit(50000 * 1e6, bob);
depositInAcc(mockERC20.stable2, aliceAccount, depositAmt);
// alice takes loan
vm.startPrank(alice);
pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0));
vm.stopPrank();
console.log("----------------After Loan----------------");
emitHelper();
// price falls down by a little
transmitOracle(mockOracles.stable2ToUsd,0.999 ether);
// assertEq(aliceAccount.isA)
assertEq(aliceAccount.isAccountLiquidatable(),true);
console.log();
console.log("----------------After PriceFall----------------");
emitHelper();
vm.startPrank(charlie);
// attack starts
liquidator.liquidateAccount(address(aliceAccount));
console.log();
console.log("----------------After AuctionStart----------------");
emitHelper();
console.log();
vm.stopPrank();
// alice pays back the debt
vm.startPrank(alice);
pool.repay(3000 * 1e6, address(aliceAccount));
vm.stopPrank();
vm.startPrank(charlie);
liquidator.endAuction(address(aliceAccount));
console.log();
console.log("----------------After AuctionEnd----------------");
emitHelper();
console.log();
uint liquidationRewards = pool.liquidityOf(charlie);
pool.withdrawFromLendingPool(liquidationRewards, charlie);
emit log_named_decimal_uint("liquidationRewards", liquidationRewards, 6);
assertGt(liquidationRewards, minimumMargin);
vm.stopPrank();
}Consider the example, Alice opens a account, currently minimumMargin in 'LendingPool.sol' = 200 $, which gets cached in her account. She deposits 400 $ and opens a position of 100 $. So getUsedMargin() for alice = 300 $ collateralValue = 400 $
Now after some time admins/riskManager update the risk parameters and minimumMargin is also updated to 300 $ . Now bob opens a Account his minimumMargin will be cached at 300 $, he deposits 410 $ collateral & opens position of 100 $ . So getUsedMargin() for bob = 400 $ collateralValue = 410 $
Assuming both of them gave the same collateral at same price. But now the collateral's value decreases by 11 $, at this point bob's position is unhealthy but alice's position is still healthy. So bob gets liquidated. Which is unfair both to bob and the LPs.
1. Both bob and alice had the same open position i.e 100 $ on the same creditor but even though bob had deposited more collateral he got liquidated first which is OK, but whats not ok is that alice's account still stays healthy. 2. This is not only unfair to Bob but by that price movement of Alice & Bob's collateral the LPs should have gained 2x Liquidation penalty profit on a Debt of 100 $ but they will only end up getting 1x Liquidation penalty which is clearly a significant loss.
Actually lets ignore the rewards for a moment,
I want to bring reader's attention to the point that there are 2 places where minimumMargin is stored .
The minimumMargin in AccountV1Storage is cached and is not updated unless a owner closes his margin account and reopens it, but minimumMargin in LendingPool.sol is dynamic . Which is problematic, this should be a standalone bug independent of my submission but it does maximize impact of my submission.
Consider the example, Alice opens a account, currently minimumMargin in 'LendingPool.sol' = 50 $, which gets cached in her account. She deposits 400 $ and opens a position of 325 $.
So getUsedMargin() for alice = 375 $
collateralValue = 400 $
Now after some time admins/riskManager update the risk parameters and minimumMargin is also updated to 70 $ . Now bob opens a Account his minimumMargin will be cached at 70 $, he deposits 410 $ collateral & opens position of 325 $ .
So getUsedMargin() for bob = 395 $
collateralValue = 410 $
Assuming both of them gave the same collateral at same price. But now the collateral's value decreases by 20 $, at this point bob's position is unhealthy but alice's position is still healthy. So bob gets liquidated. Which is unfair both to bob and the LPs.
I have explained this bug in previous comment but there the values of minimumMargin is in 100s $ and changes by 100s $ of dollar which i guess does not work out with sponsors but i was only trying to keep things simple.
All this happens because of a state mismatch and not because of incorrect admin input Pair this with multiple upgrades to minimumMargin in LendingPool.sol and we will have a contract state where some accounts get liquidated even though they have bigger collateral value & similar positions but others will not.
I have given its fix in the comment as well i.e to not cache the value of minimumMargin in AccountV1 at all instead fetch it dynamically from the creditor contract. There will be no looping over all accounts involved in that case.
I also want to notify that all other risk parameters are fetched dynamically in the when liquidating only stale and now invalid minimumMargin is fetched from AccountV1. Which is why i say that this maximizes the impact of my original finding.
The cachingProblem poc now with updated and more restricted minimumMargin.
function testCachingProblem2() public {
uint256 minimumMargin = 50 * 1e6;
uint depositAmt = 400 * 1e6; //depositAmt of mockStable2
uint openPosition = 325 * 1e6;
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(minimumMargin));
vm.stopPrank();
// setting riskParams
vm.startPrank(users.riskManager);
registryExtension.setRiskParametersOfPrimaryAsset(
address(pool),
address(mockERC20.stable2),
0,
type(uint112).max,
uint16(AssetValuationLib.ONE_4),
uint16(AssetValuationLib.ONE_4)
);
vm.stopPrank();
// checking minimum margin set
(,,,uint minimumMargin_) = pool.openMarginAccount(1);
assertEq(minimumMargin,minimumMargin_);
// alice creates Account
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
// depositing in jr tranche
mockERC20.stable1.mint(address(this),50000 * 1e6);
mockERC20.stable1.approve(address(pool), 50000 * 1e6);
jrTranche.deposit(50000 * 1e6, bob);
depositInAcc(mockERC20.stable2, aliceAccount, depositAmt);
// alice takes loan
vm.startPrank(alice);
pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0));
vm.stopPrank();
console.log("----------------After Loan----------------");
emit log_named_decimal_uint("openPosAlice", pool.getOpenPosition(address(aliceAccount)), 6);
emit log_named_decimal_uint("getUsedMarginAlice",aliceAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueAlice",aliceAccount.getCollateralValue(),6);
console.log();
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(70 * 1e6));
vm.stopPrank();
vm.startPrank(bob);
bobAccount = AccountV1(factory.createAccount(5678, 0, address(pool)));
vm.stopPrank();
depositInAcc(mockERC20.stable2, bobAccount, 410 * 1e6);
// alice takes loan
vm.startPrank(bob);
pool.borrow(openPosition, address(bobAccount), bob, bytes3(0));
vm.stopPrank();
console.log("----------------After Bob Loan----------------");
emit log_named_decimal_uint("openPosBob", pool.getOpenPosition(address(bobAccount)), 6);
emit log_named_decimal_uint("getUsedMarginBob",bobAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueBob",bobAccount.getCollateralValue(),6);
console.log();
transmitOracle(mockOracles.stable2ToUsd,0.95 ether);
assertEq(bobAccount.isAccountLiquidatable() , true);
assertEq(bobAccount.isAccountUnhealthy(),true);
assertEq(aliceAccount.isAccountLiquidatable(), false);
assertEq(aliceAccount.isAccountUnhealthy() , false);
console.log("----------------At End----------------");
emit log_named_decimal_uint("openPosAlice", pool.getOpenPosition(address(aliceAccount)), 6);
emit log_named_decimal_uint("getUsedMarginAlice",aliceAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueAlice",aliceAccount.getCollateralValue(),6);
emit log_named_decimal_uint("openPosBob", pool.getOpenPosition(address(bobAccount)), 6);
emit log_named_decimal_uint("getUsedMarginBob",bobAccount.getUsedMargin(),6);
emit log_named_decimal_uint("collValueBob",bobAccount.getCollateralValue(),6);
}There have been some misunderstandings i think, the bug is not about minRewards being greater than maxReward
setLiquidationParameters quotes "Sum of the initiationReward and terminationReward cannot exceed minimumMargin of the Account. hence minRewardWeight is capped to 50%."
There indeed was misunderstanding. The comment:
// Sum of the initiationReward and terminationReward cannot exceed minimumMargin of the Account.
Is of course only valid when the minimumMargin is the determining factor for the rewards (for dust positions).
For all other positions it are the collateral assets themselves that pay for the liquidations.
While i agree that that comment is formulated not very clear, the code behaves as intended. So the only fix is to better formulate the comment.
Kalyan-Singh
commented
6 months ago So my closing statement on this issue would be-
I have kept every single parameter in sponsor's said constraints in my latter POC & in the smart contract's acceptable constraints i.e-
I ask lead judge and sherlock judge to cross verify and check what 'bogus' input did I give?
Now that i have proved that this is not a admin input error, sponsors again have a misunderstanding that this is a code comment error, with which i respectfully disagree.
I have clearly shown in the POCs how LPs loose yield, which does not count as code comment error.
Also the caching of minimumMargin in each account's local storage is a much more serious issue(which i think has been missed during the original audit time period), which leads to many borrowers which opened margin account at a lower value of minimumMargin not get liquidated when they should get liquidated. This is wrong on so many aspects-
I want sherlock judge and lead judge to factor in all these points while evaluating. Boosted & re-affirmed by the standalone finding i now say that the finding is a borderline high severity.
And I request sponsors to look at the POCs again if possible (i apologize if you think i wasted time) , but I think I have given sufficient details on why this is neither a Admin input error nor a code comment issue but is a geniune loss of yield due to state mismatch.
Thanks for reading!
Thomas-Smets
commented
6 months ago I just ran your POC with the following code for emitHelper()
function emitHelper() public {
emit log_named_uint("openPosAlice", pool.getOpenPosition(address(aliceAccount)));
emit log_named_uint("totalLiquidity", pool.totalLiquidity());
}As you can see, LPs do not loose funds, contrary, they get the full rewards:
I think you misunderstand the complete liquidation flow, the liquidation rewards are added to the debt of the borrower at the beginning of the liquidation, see https://github.com/arcadia-finance/lending-v2/blob/dcc682742949d56928e7e8e281839d2229bd9737/src/LendingPool.sol#L879 Or see how Alices open position increases after the auction is started.
In you example Alice next pays enough debt to bring her Account back in a healthy position. The LPs receive in this case the full rewards.
From your own example (with the correct emits) you can see that it is borrower who pays for the liquidation rewards, not the LPs! If we assume there are no gas costs, we would not even need a minimumMargin and minimumReward, since we use a liquidationFactor that can in all case can cover the penalties, all debt + liquidation rewards can be paid out from the collateral assets of the borrower!
Taking into account gas cost, we do get a problem for small positions tho, now next to the liquidation rewards, we also need to make sure that we can cover the gas costs of the liquidators. Now the assumption that via the liquidationFactor we will have enough assets to cover all debt + rewards does not hold. For the very small positions we need some additional collateral to pay for the gas costs. That is the minimumMargin.
I hope you now understand what it is, and that you also see that minimumMargin is only important to also liquidate efficiently very small positions. Having a to small minimumMargin does not lead to immediately losses for LPs, it would just result in some dust positions not being liquidated efficiently.
Now to come to your second point:
Also the caching of minimumMargin in each account's local storage is a much more serious issue
There is no state mismatch, all logic of for Liquidations (even in the LndingPool) use the stored (not cashed) value in the Account.
And secondly now that I hope you understand the purpose of minimumMargin, it is just a value to prevent dust positions.
It is not as crucial to enforce solvability of a position as say the liquidation- and collateralFactor.
It works perfectly well stored on the Account.
There is a good reason (future feature related) why we store it in each account and not in the Registry, but I will not go into these details, since the reason why is not relevant for this issue.
Kalyan-Singh
commented
6 months ago Sir, I agree that the case I gave in POC is a case where alice(borrower) repays back the penalty + much more so the LPs were not impacted but that is the most optimal scenario. That POC was only to clear your misunderstandings about my finding being based on admin input error & clear the fact that i am not stating minReward > maxReward.
Now consider the scenario where alice does not repay and is happy with her loaned amount.(which is a 50% probablity, coz only 2 cases exist a. alice repays , b. alice does not repay)
I did not show the case b. because i did not think it was required to make things clear and it requires a bit more ninja tricks to simulate a dutch auction in foundry tests. But now i shall provide you the POC of case b., in this case the LPs loose funds.
So, since alice does not repay, dutch auction continues, everyone waits till dutch auction price reaches the current market value of collateral or below it ( generally in dutch auctions collateral is bought at discount of a few $ value) . The price reaches ~24975 $ which is the collateral value of alice (in the poc its 24973 to be exact , a 2 $ discount is not absurd, simulating dutch auction to reach exact price in foundry test is very hard)
----------------After AuctionStart---------------- openPos: 26571.750000 getUsedMargin: 26621.750000 collValue: 24975.000000 //@audit col value of 24975$
so now charlie places the bid and buys out the entire collateral, in this case, alice(borrower) did not pay for the liquidator rewards, neither did charlie(liquidator). To make things clear lets look at full state.
----------------After AuctionStart---------------- openPos: 26571.750000 getUsedMargin: 26621.750000 collValue: 24975.000000 realizedLiqBenifit: 249.500000 totalLiq: 50249.500000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After AuctionEnd---------------- openPos: 0.000000 getUsedMargin: 50.000000 collValue: 0.000000 realizedLiqBenifit: 249.500000 //@audit <-benifit of charlie totalLiq: 50022.565357 stable1AliceBalance: 24950.000000 JrTr: 49773.065357 //@audit <- loss of 27 $ to LPs
As you can see LPs lost 27 $, charlie benifited 249.5 $ but now twist charlie = alice & alice = charlie, they are just sybil identities,
so in a way alice gain = 24975 $ + 249.5 $ = 25,224.5 $ , so now alice the borrower gained a sizeable profit i.e 224.5 $ extra even than her original collateral value. while LPs loose 27 $.
Here is the poc -
function testExtraLiqRewards2() public {
uint256 minimumMargin = 50 * 1e6;
uint depositAmt = 25000 * 1e6; //depositAmt of mockStable2
uint openPosition = 24950 * 1e6;
// depositing amount into charlie
// mockERC20.stable1.mint(charlie,200 * 1e6) ;
assertEq(mockERC20.stable1.balanceOf(charlie),0);
vm.prank(alice);
mockERC20.stable1.approve(address(pool),UINT256_MAX);
{
vm.startPrank(pool.owner());
(uint16 initiationW , uint16 penaltyW , uint16 terminationW , uint16 minRewardWeight , uint80 maxReward)=pool.getLiquidationParameters();
// setting max reward to 1000$ and minReward weight to 2%
pool.setLiquidationParameters(initiationW, penaltyW, terminationW, 1000, 1000 * 1e6);
emit log_named_uint("penaltyW", penaltyW);
emit log_named_uint("initiationW", initiationW);
emit log_named_uint("terminationW", terminationW);
vm.stopPrank();
vm.startPrank(users.riskManager);
registryExtension.setRiskParametersOfPrimaryAsset(
address(pool),
address(mockERC20.stable2),
0,
type(uint112).max,
uint16(AssetValuationLib.ONE_4),
uint16(AssetValuationLib.ONE_4)
);
vm.stopPrank();
}
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(minimumMargin));
vm.stopPrank();
// checking minimum margin set
(,,,uint minimumMargin_) = pool.openMarginAccount(1);
assertEq(minimumMargin,minimumMargin_);
// alice creates Account
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
// depositing in jr tranche
mockERC20.stable1.mint(address(this),50000 * 1e6);
mockERC20.stable1.approve(address(pool), 50000 * 1e6);
jrTranche.deposit(50000 * 1e6, bob);
depositInAcc(mockERC20.stable2, aliceAccount, depositAmt);
// alice takes loan
vm.startPrank(alice);
pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0));
vm.stopPrank();
console.log("----------------After Loan----------------");
emitHelper();
// price falls down by a little
transmitOracle(mockOracles.stable2ToUsd,0.999 ether);
// assertEq(aliceAccount.isA)
assertEq(aliceAccount.isAccountLiquidatable(),true);
console.log();
console.log("----------------After PriceFall----------------");
emitHelper();
vm.startPrank(charlie);
// attack starts
liquidator.liquidateAccount(address(aliceAccount));
console.log();
console.log("----------------After AuctionStart----------------");
emitHelper();
console.log();
vm.stopPrank();
// alice does not pay back the debt
// so , in dutch auction everyone tries to buy at max at the collateral's face value ie. 24975 $
// but in maximum cases collateral will be bought at a small discount i.e less than 24975 $
// so here charlie buys at ~24973 $ , a 2 $ discount which again is not absurd
vm.warp(block.timestamp + 1 hours + 10 minutes );
assertEq(mockERC20.stable1.balanceOf(charlie),0);
mockERC20.stable1.mint(charlie, 24973 * 1e6);
vm.startPrank(charlie);
mockERC20.stable1.approve(address(pool), 24973 * 1e6);
uint[] memory assetAmts = new uint[](1);
assetAmts[0] = 25000 * 1e6;
liquidator.bid(address(aliceAccount), assetAmts, true);
vm.stopPrank();
vm.startPrank(charlie);
// liquidator.endAuction(address(aliceAccount));
console.log();
console.log("----------------After AuctionEnd----------------");
emitHelper();
console.log();
uint liquidationRewards = pool.liquidityOf(charlie);
pool.withdrawFromLendingPool(liquidationRewards, charlie);
emit log_named_decimal_uint("liquidationRewards", liquidationRewards, 6);
assertGt(liquidationRewards, minimumMargin);
vm.stopPrank();
}also even if alice != charlie the fact remains the LPs loose funds.
Kalyan-Singh
commented
6 months ago There is a state mismatch sir,
function testCachingProblem3() public {
uint96 minimumMargin1 = 50 * 1e6;
uint96 minimumMargin2 = 55 * 1e6;
vm.startPrank(pool.owner());
pool.setMinimumMargin(minimumMargin1);
vm.stopPrank();
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
vm.startPrank(pool.owner());
pool.setMinimumMargin(minimumMargin2);
vm.stopPrank();
vm.startPrank(bob);
bobAccount = AccountV1(factory.createAccount(5678, 0, address(pool)));
vm.stopPrank();
require(bobAccount.minimumMargin() != aliceAccount.minimumMargin());
}
The mismatch is between minimumMargin of accounts & minimumMargin of LendingPool.sol, when the risk manager updates the minimumMargin, the change only reflects in LendingPool.sol not in AccountsV1.sol, but as you correctly understand the stored value of AccountsV1 is used everywhere. (Here the bobAccount has latest value of minimumMargin according to LendingPool.sol)
And i have already explained how it is harmful to the LPs with poc i.e accounts do not get liquidated when they should get liquidated. I understand that storing minimumMargin might be important but at least fetch that value dynamically from creditor, in critical functions such as getUsedMargin() and maybe update it if the stored value != latest fetched value.
j-vp
commented
6 months ago Sir, I agree that the case I gave in POC is a case where alice(borrower) repays back the penalty + much more so the LPs were not impacted but that is the most optimal scenario. That POC was only to clear your misunderstandings about my finding being based on admin input error & clear the fact that i am not stating minReward > maxReward.
Now consider the scenario where alice does not repay and is happy with her loaned amount.(which is a 50% probablity, coz only 2 cases exist a. alice repays , b. alice does not repay)
I did not show the case b. because i did not think it was required to make things clear and it requires a bit more ninja tricks to simulate a dutch auction in foundry tests. But now i shall provide you the POC of case b., in this case the LPs loose funds.
So, since alice does not repay, dutch auction continues, everyone waits till dutch auction price reaches the current market value of collateral or below it ( generally in dutch auctions collateral is bought at discount of a few $ value) . The price reaches ~24975 $ which is the collateral value of alice (in the poc its 24973 to be exact , a 2 $ discount is not absurd, simulating dutch auction to reach exact price in foundry test is very hard)
----------------After AuctionStart----------------
openPos: 26571.750000
getUsedMargin: 26621.750000
collValue: 24975.000000 //@audit col value of 24975$
so now charlie places the bid and buys out the entire collateral, in this case, alice(borrower) did not pay for the liquidator rewards, neither did charlie(liquidator). To make things clear lets look at full state.
----------------After AuctionStart----------------
openPos: 26571.750000
getUsedMargin: 26621.750000
collValue: 24975.000000
realizedLiqBenifit: 249.500000
totalLiq: 50249.500000
stable1AliceBalance: 24950.000000
JrTr: 50000.000000
----------------After AuctionEnd----------------
openPos: 0.000000
getUsedMargin: 50.000000
collValue: 0.000000
realizedLiqBenifit: 249.500000 //@audit <-benifit of charlie
totalLiq: 50022.565357
stable1AliceBalance: 24950.000000
JrTr: 49773.065357 //@audit <- loss of 27 $ to LPs
As you can see LPs lost 27 $, charlie benifited 249.5 $ but now twist charlie = alice & alice = charlie, they are just sybil identities,
so in a way alice gain = 24975 $ + 249.5 $ = 25,224.5 $ , so now alice the borrower gained a sizeable profit i.e 224.5 $ extra even than her original collateral value. while LPs loose 27 $.
Here is the poc -
function testExtraLiqRewards2() public { uint256 minimumMargin = 50 * 1e6; uint depositAmt = 25000 * 1e6; //depositAmt of mockStable2 uint openPosition = 24950 * 1e6; // depositing amount into charlie // mockERC20.stable1.mint(charlie,200 * 1e6) ; assertEq(mockERC20.stable1.balanceOf(charlie),0); vm.prank(alice); mockERC20.stable1.approve(address(pool),UINT256_MAX); { vm.startPrank(pool.owner()); (uint16 initiationW , uint16 penaltyW , uint16 terminationW , uint16 minRewardWeight , uint80 maxReward)=pool.getLiquidationParameters(); // setting max reward to 1000$ and minReward weight to 2% pool.setLiquidationParameters(initiationW, penaltyW, terminationW, 1000, 1000 * 1e6); emit log_named_uint("penaltyW", penaltyW); emit log_named_uint("initiationW", initiationW); emit log_named_uint("terminationW", terminationW); vm.stopPrank(); vm.startPrank(users.riskManager); registryExtension.setRiskParametersOfPrimaryAsset( address(pool), address(mockERC20.stable2), 0, type(uint112).max, uint16(AssetValuationLib.ONE_4), uint16(AssetValuationLib.ONE_4) ); vm.stopPrank(); } vm.startPrank(pool.owner()); pool.setMinimumMargin(uint96(minimumMargin)); vm.stopPrank(); // checking minimum margin set (,,,uint minimumMargin_) = pool.openMarginAccount(1); assertEq(minimumMargin,minimumMargin_); // alice creates Account vm.startPrank(alice); aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool))); vm.stopPrank(); // depositing in jr tranche mockERC20.stable1.mint(address(this),50000 * 1e6); mockERC20.stable1.approve(address(pool), 50000 * 1e6); jrTranche.deposit(50000 * 1e6, bob); depositInAcc(mockERC20.stable2, aliceAccount, depositAmt); // alice takes loan vm.startPrank(alice); pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0)); vm.stopPrank(); console.log("----------------After Loan----------------"); emitHelper(); // price falls down by a little transmitOracle(mockOracles.stable2ToUsd,0.999 ether); // assertEq(aliceAccount.isA) assertEq(aliceAccount.isAccountLiquidatable(),true); console.log(); console.log("----------------After PriceFall----------------"); emitHelper(); vm.startPrank(charlie); // attack starts liquidator.liquidateAccount(address(aliceAccount)); console.log(); console.log("----------------After AuctionStart----------------"); emitHelper(); console.log(); vm.stopPrank(); // alice does not pay back the debt // so , in dutch auction everyone tries to buy at max at the collateral's face value ie. 24975 $ // but in maximum cases collateral will be bought at a small discount i.e less than 24975 $ // so here charlie buys at ~24973 $ , a 2 $ discount which again is not absurd vm.warp(block.timestamp + 1 hours + 10 minutes ); assertEq(mockERC20.stable1.balanceOf(charlie),0); mockERC20.stable1.mint(charlie, 24973 * 1e6); vm.startPrank(charlie); mockERC20.stable1.approve(address(pool), 24973 * 1e6); uint[] memory assetAmts = new uint[](1); assetAmts[0] = 25000 * 1e6; liquidator.bid(address(aliceAccount), assetAmts, true); vm.stopPrank(); vm.startPrank(charlie); // liquidator.endAuction(address(aliceAccount)); console.log(); console.log("----------------After AuctionEnd----------------"); emitHelper(); console.log(); uint liquidationRewards = pool.liquidityOf(charlie); pool.withdrawFromLendingPool(liquidationRewards, charlie); emit log_named_decimal_uint("liquidationRewards", liquidationRewards, 6); assertGt(liquidationRewards, minimumMargin); vm.stopPrank(); }also even if alice != charlie the fact remains the LPs loose funds.
Your example starts wrong. You take an open position of 26571 against a collateral value of 24975. In that case, the position is already way undercollateralized.
Instead, the positions' market value should be eg 32000, with a collateral value of 30000, a liquidation value of 28000 and an open position of 26571 (fictive numbers). You already assume that the open position is higher than the market value of the collateral, which should be "3 levels" (liq, col, market value) lower than the open position.
Kalyan-Singh
commented
6 months ago No sir the e.g does not 'start' with an underwater position only the log's start, the e.g starts with collateral of 25k and open pos of 24950 $ , the logs that I pasted in that comment start after the liquidation has already started here as tsmets said the extra debt has been minted.
Here are the full Logs, did not paste completely in last comment coz they too long Logs: penaltyW: 500 initiationW: 100 terminationW: 50 ----------------After Loan---------------- openPos: 24950.000000 //@Audit <- openPosition getUsedMargin: 25000.000000 collValue: 25000.000000 //@Audit <- collValue at start realizedLiqBenifit: 0.000000 totalLiq: 50000.000000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After PriceFall---------------- openPos: 24950.000000 getUsedMargin: 25000.000000 collValue: 24975.000000 realizedLiqBenifit: 0.000000 totalLiq: 50000.000000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After AuctionStart---------------- openPos: 26571.750000 getUsedMargin: 26621.750000 collValue: 24975.000000 realizedLiqBenifit: 249.500000 totalLiq: 50249.500000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After AuctionEnd---------------- openPos: 0.000000 getUsedMargin: 50.000000 collValue: 0.000000 realizedLiqBenifit: 249.500000 totalLiq: 50022.565357 stable1AliceBalance: 24950.000000 JrTr: 49773.065357
Again the e.g starts fine 25000 $ collarteral value at a open Position of 24950 $, it goes to 26571 $ after the auction started and extra debt with penalty is minted.
Thomas-Smets
commented
6 months ago You do not take into account two factors in your example:
startPriceMultiplier: https://github.com/arcadia-finance/lending-v2/blob/dcc682742949d56928e7e8e281839d2229bd9737/src/Liquidator.sol#L355Here is an example where we take those into account, and for now we ignore gas costs.
penaltyW: 500 initiationW: 100 terminationW: 50 collFactor: 0.8 liqFactor: 0.9 startPriceMultiplier: 30% ----------------After Loan---------------- openPos: 24950.000000 getUsedMargin: 25000.000000 collValue: 25000.000000 liqValue: 28125 assetValue: 31250 realizedLiqBenifit: 0.000000 totalLiq: 50000.000000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After PriceFall---------------- openPos: 24950.000000 getUsedMargin: 25000.000000 collValue: 22200.000000 liqValue: 24975 assetValue: 27750 initiator: 0 terminator: 0 totalLiq: 50000.000000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After AuctionStart---------------- openPos: 26571.750000 getUsedMargin: 26621.750000 collValue: 22200.000000 liqValue: 24975 assetValue: 27750 auctionPrice: 36075 initiator: 249.500000 terminator: 0 totalLiq: 50249.500000 stable1AliceBalance: 24950.000000 JrTr: 50000.000000
----------------After AuctionEnd---------------- openPos: 0.000000 getUsedMargin: 50.000000 collValue: 0 liqValue: 0 assetValue: 0 auctionPrice: 27750 (auctionPrice == assetValue) initiator: 249.500000 terminator: 124.75 totalLiq: 52800 stable1AliceBalance: 24950.000000 JrTr: 51247.5 surplusAlice: 1178.25
As you can see from the example it is not the minimumMargin that ensures there is enough collateral to pay the rewards/penalties. It is the liquidationFactor that ensures that when auctions are triggered, there are enough assets left to pay back both the openPosition and the liquidation rewards/penalties.
This is also what I tried to explain in my comment in https://github.com/sherlock-audit/2023-12-arcadia-judging/issues/162#issuecomment-1973555835
Since both the penalties/rewards and the liquidation reward are defined as a percentage of the openPosition and if we ignore gas costs the liquidationFactor alone would be sufficient to pay for the liquidation fees/rewards and we would not need a minimumMargin at all!
Now if we take gas costs into account, the story is slightly different, since both the initiator as the actual bidders who do the liquidations must recover at least there gas costs.
This has as consequence that the initiator needs to receive an initiatorReward that is at least as big as his gas costs.
And that the final auctionPrice will be below the assetValue (in efficient markets it will be: assetValue - gasPrice).
The problem is that gas costs do not depend on the openPosition, so for small positions the liquidationFactor (which depends on position size) is not sufficient to ensure at least gas costs are covered.
That is why we need a minimumMargin to ensure that there enough asset to cover the gas costs for these small positions.
For normal and big positions, the liquidationFactor ensures there is enough collateral in the Account to cover the the gas costs. But only for small dust positions that is not sufficient, and that is why we need the minimumMargin.
The logic is implemented as intended, this i can further proof with the audit report of Renascene, which was in scope as a supporting supporting document (see L-8): https://github.com/arcadia-finance/arcadia-finance-audits/blob/main/audits-v2/RENASCENCE-BYTES-HOLLADIEWALDFEE-ALEX_Q12024.pdf (note that the term fixedLiquidationCost was used here instead of minimumMargin).
To come back what I said earlier, I think the confusion lies in the comments in the code, which indeed should be clarified. Not in the logic itself.
Kalyan-Singh
commented
6 months ago Yes sir, you are right. But I do not understand why you said SPM = 30 % when SPM's( 4 decimal precision) limits are hard-coded to be between 100% to 300% as can be verified here & here .
Now, even with the above scenario, in edge cases the borrower can end up in profit even with liquidations. Here is the coded POC(here interest rate is used to make account liquidatable as a direct price change would require a collateral price to drop down to 0.88 $ which is obviously a case of de-pegging/ non stable which is not realistic) -
function testExtraLiqRewards3() public {
{
vm.startPrank(pool.owner());
(uint16 initiationW , uint16 penaltyW , uint16 terminationW , uint16 minRewardWeight , uint80 maxReward)=pool.getLiquidationParameters();
// setting max reward to 1000$ and minReward weight to 2%
pool.setLiquidationParameters(initiationW, penaltyW, terminationW, 1000, 1000 * 1e6);
emit log_named_uint("penaltyW", penaltyW);
emit log_named_uint("initiationW", initiationW);
emit log_named_uint("terminationW", terminationW);
vm.stopPrank();
vm.startPrank(users.riskManager);
registryExtension.setRiskParametersOfPrimaryAsset(
address(pool),
address(mockERC20.stable2),
0,
type(uint112).max,
uint16(8000),
uint16(9000)
);
vm.stopPrank();
}
uint72 base = 0;
uint72 low = 3 ether;
uint72 high = 3 ether;
uint16 ut= 6500;
uint256 minimumMargin = 50 * 1e6;
uint depositAmt = 25000 * 1e6; //depositAmt of mockStable2
uint openPosition = 19950 * 1e6;
//setting interest rates
vm.prank(pool.owner());
pool.setInterestParameters(base,low,high,ut);
// depositing amount into charlie
// mockERC20.stable1.mint(charlie,200 * 1e6) ;
assertEq(mockERC20.stable1.balanceOf(charlie),0);
vm.prank(alice);
mockERC20.stable1.approve(address(pool),UINT256_MAX);
vm.startPrank(pool.owner());
pool.setMinimumMargin(uint96(minimumMargin));
vm.stopPrank();
// checking minimum margin set
(,,,uint minimumMargin_) = pool.openMarginAccount(1);
assertEq(minimumMargin,minimumMargin_);
// alice creates Account
vm.startPrank(alice);
aliceAccount = AccountV1(factory.createAccount(1234, 0, address(pool)));
vm.stopPrank();
// depositing in jr tranche
mockERC20.stable1.mint(address(this),50000 * 1e6);
mockERC20.stable1.approve(address(pool), 50000 * 1e6);
jrTranche.deposit(50000 * 1e6, bob);
depositInAcc(mockERC20.stable2, aliceAccount, depositAmt);
// alice takes loan
vm.startPrank(alice);
emit log_named_decimal_uint("free margin",aliceAccount.getFreeMargin(),6);
pool.borrow(openPosition, address(aliceAccount), alice, bytes3(0));
vm.stopPrank();
console.log("----------------After Loan----------------");
emitHelper();
vm.startPrank(users.defaultTransmitter);
// price falls down by a little
vm.warp(block.timestamp+ 52 days + 70 hours);
mockOracles.stable1ToUsd.transmit(1 ether);
mockOracles.stable2ToUsd.transmit(0.999 ether);
pool.syncInterests();
vm.stopPrank();
// assertEq(aliceAccount.isA)
assertEq(aliceAccount.isAccountLiquidatable(),true);
console.log();
console.log("----------------After PriceFall----------------");
emitHelper();
vm.startPrank(charlie);
// attack starts
liquidator.liquidateAccount(address(aliceAccount));
console.log();
console.log("----------------After AuctionStart----------------");
emitHelper();
console.log();
vm.stopPrank();
// alice does not pay back the debt
// so , in dutch auction everyone tries to buy at max at the collateral's face value ie. 24975 $
// but in maximum cases collateral will be bought at a small discount i.e less than 24975 $
// so here charlie buys at ~24973 $ , a 2 $ discount which again is not absurd
vm.warp(block.timestamp + 48 minutes + 50 seconds);
assertEq(mockERC20.stable1.balanceOf(charlie),0);
mockERC20.stable1.mint(charlie, 24973 * 1e6);
vm.startPrank(charlie);
mockERC20.stable1.approve(address(pool), 24973 * 1e6);
uint[] memory assetAmts = new uint[](1);
assetAmts[0] = 25000 * 1e6;
liquidator.bid(address(aliceAccount), assetAmts, true);
require(mockERC20.stable1.balanceOf(charlie) < 1 * 1e6);
vm.stopPrank();
vm.startPrank(charlie);
// liquidator.endAuction(address(aliceAccount));
console.log();
console.log("----------------After AuctionEnd----------------");
emitHelper();
console.log();
uint liquidationRewards = pool.liquidityOf(charlie);
pool.withdrawFromLendingPool(liquidationRewards, charlie);
emit log_named_decimal_uint("liquidationRewards", liquidationRewards, 6);
assertGt(liquidationRewards, minimumMargin);
vm.stopPrank();
}Here are the logs - penaltyW: 500 initiationW: 100 terminationW: 50 free margin: 19950.000000 ----------------After Loan---------------- openPos: 19950.000000 getUsedMargin: 20000.000000 collValue: 20000.000000 liquidationValue: 22500.000000 realizedLiqBenifit: 0.000000 aliceSurplus: 0.000000 totalLiq: 50000.000000 stable1AliceBalance: 19950.000000 JrTr: 50000.000000
----------------After PriceFall---------------- openPos: 22458.125030 getUsedMargin: 22508.125030 collValue: 19980.000000 liquidationValue: 22477.500000 realizedLiqBenifit: 0.000000 aliceSurplus: 0.000000 totalLiq: 52508.125030 stable1AliceBalance: 19950.000000 JrTr: 51003.250012
----------------After AuctionStart---------------- openPos: 23917.903157 getUsedMargin: 23967.903157 collValue: 19980.000000 liquidationValue: 22477.500000 realizedLiqBenifit: 224.581250 aliceSurplus: 0.000000 totalLiq: 52732.706280 stable1AliceBalance: 19950.000000 JrTr: 51003.250012
----------------After AuctionEnd---------------- openPos: 0.000000 getUsedMargin: 50.000000 collValue: 0.000000 liquidationValue: 0.000000 realizedLiqBenifit: 336.871875 //@Audit <- initiation & termination reward aliceSurplus: 1052.764868 //@Audit <- surplus to alice totalLiq: 55022.576694 stable1AliceBalance: 19950.000000 JrTr: 51228.594729
liquidationRewards: 336.871875
So here if alice(borrower) is the liquidator(charlie), in the end her net profits would be -
She put in 25000 $ of collateral initially, now the collateral is worth 24975 $. She buys back the collateral of 24975$ (at a 2 $ discount) + gains 1052.76 $ surplus + gets 336.87 $ of rewards = 26363 $ these extra 1363 $ are still theoretically being paid out from the yield of LPs, while alice who should have lost money through the liquidation mechanism walks away with profit, which is clearly cheating.
Aside from this edge case, some more points i would like to make to sherlock judge -
// Sum of the initiationReward and terminationReward cannot exceed minimumMargin of the Account.
// -> minRewardWeight is capped to 50%.But yes now I do not think this is a high, this should at best be contemplated for a medium.
Thankss for your time.
j-vp
commented
6 months ago I think I found the issue; you forget that she starts the liquidation at a -$2500 net balance (the difference in market value of the collateral she put in, and what she borrowed against it).
I'll work with these numbers, assuming market value is still around $25000 at the time of liquidation.
Here are the logs - penaltyW: 500 initiationW: 100 terminationW: 50 minimumMargin: 50
Right before liquidation:
openPos: 22458.125030 getUsedMargin: 22508.125030 collValue: 19980.000000 liquidationValue: 22477.500000 realizedLiqBenifit: 0.000000 aliceSurplus: 0.000000 totalLiq: 52508.125030 stable1AliceBalance: 19950.000000 JrTr: 51003.250012
Right after liquidation started:
openPos: 23917.903157 getUsedMargin: 23967.903157 collValue: 19980.000000 liquidationValue: 22477.500000 realizedLiqBenifit: 224.581250 aliceSurplus: 0.000000 totalLiq: 52732.706280 stable1AliceBalance: 19950.000000 JrTr: 51003.250012
The numbers: initiationReward = 22458 100 / 10000 = 224.58 terminationReward = 22458 50 / 10000 = 112.29 liquidationPenalty = 22458 * 500 / 10000 = 1122.9
Total open position when auction starts = 23917.9 Total profit of initiator: 224.58
Auction gets fully bought and ended at $24975
surplus = $1058 -> _settleLiquidationHappyFlow will be triggered _syncLiquidationFee will sync $1122.9 to the junior tranche $1058 will be added to Alice's liquidity
Now Alice, who had an open position of $22458 (not $23917 (!)), had to repay $23917 through the sale of her collateral, which yielded $24975 in total. The surplus that ended up, is given back to her as liquidity in the lending pool.
Let's rephrase the scenario. Alice puts in $25k as collateral and borrows out $22500 in usdc, which she now has in her wallet (not the account, just a normal borrow for simplicity). Her net balance is now -$2500. She gets liquidated, and gets $1000 back in total. Her total balance is now -$1500: she thus paid a total fee of $1500.
Other scenario: Alice puts in $25k as collateral and borrows $22.5k usdc. Her net balance is now -$2500. She liquidates herself, bids on herself for $24975 and terminates it herself. In total, the cost of this was: $25k + $24975 = $49975 However she still gets things back: collateral worth $25k and $336 in initiation + termination fees. She also has $1000 in surplus.
-> total balance: -49975 + 25000 + 336 + 22500 + 1000 = -1139. She thus still ended up with -1139 less than the start. This deficit is smaller than in the scenario above, but still a net negative loss for her (the 1139 went to the LPs).
Other scenario: Alice puts in $25k as collateral and borrows $22.5k usdc. Her net balance is now -$2500. She liquidates herself, uses the $22.5k she borrowed + another $2475 from her own pocket to buy back her collateral and terminates it herself. In total, the cost of this was: $2475 However she still gets things back: $336 in initiation + termination fees. She also has $1000 in surplus.
The end result always remains the same: LPs get aprox 1139 in fees. The initiator gets $224, whoever it is. The terminator gets $112, whoever it is. Alice gets back $1000. Alice pays approx $1500 in fees.
She buys back the collateral of 24975$ (at a 2 $ discount) + gains 1052.76 $ surplus + gets 336.87 $ of rewards = 26363 $
This sentence is not correct, you forget the -25k cost and +22.5k from the borrow, which makes a big difference! Essentially you forget to take into account the complete value difference of the collateral factor!
Kalyan-Singh
commented
6 months ago No sir, I think you are confusing between borrowing mechanism and liquidating mechanism. Let's keep things simple
Alice put up 25000 $ (25000 1e6 tokens) worth collateral in Account. Now she is being liquidated when her collateral is worth 24975 $ . According to current code dutch auctions work on her entire Account worth i.e 24975 $ not 22.5k , she being the liquidator buys back all the collateral tokens 25000 1e6 at 24972 $
Hence 24975$ (at a 2 $ discount) + gains 1052.76 $ surplus + gets 336.87 $ of rewards = 26363 $ so she goes in a net profit of 1363 $
Some log screenshots -
Kalyan-Singh
commented
6 months ago Nevermind I am an idiot , this issue is invalid, the only thing worth was the code comment error. Sorry for wasting time.
Czar102
commented
6 months ago Planning to reject the escalation and leave the issue as is.
Czar102
commented
6 months ago Result: Invalid Duplicate of #1
sherlock-admin2
commented
6 months ago
Kalyan-Singh
medium
MinRewardWeight does not behave as intended and can lead to extra reward payout than minimum margin
Summary
minRewardWeight is the parameter used by the protocol to make sure that liquidation intiation and termination reward's sum do not exceed minimum margin, it is capped to a max of 5000(i.e 50%) but it might not behave as intended as there are insufficient checks on the maxRewards parameter & on the _calculateRewards function .
Vulnerability Detail
Lets try to understand the vuln better by walking through a scenario- In Lending pool penaltyWeight = 500, initiationWeight= 100, terminationWeight = 50 & minRewardsWeight = 200(2%, way below the 50% mark), initialMargin = 200 usd(more than enough to pay for gas fees on L2s) & maxReward = 200$.
Alice has 25000$ in collateral, 24800$ in openPosition, the price of alice's collateral moves a little downwards, lets say from 1 usd to 0.999 usd. Now alice has become liquidatble. Charlie starts the liquidation so _calculateRewards will calculate the initiationReward as follows (assuming both the lending and collateral have 6 decimals)-
minReward = (200e6 * 200) / 10000 = 4e6(~4 $) initiationReward = (24800e6 * 100) / 10000 = 248e6 (248$) now since initiationReward > maxReward(200 $) it will set initiationReward = 200 $ which in itself is equal to minimumMargin. similarly terminationReward will be set to 124 $. So a totalLiquidation reward of 324$ which is leads to extra than intended.
By the comments of hyperlink given in the summary, judge can verify that the developers are under the said assumption which is incorrect as i proved in above example
Here is a working poc -
add it in lending-v2/test/fuzz/LendingPool as a new file. and run using
Impact
Extra rewards than intended, loss to LPs
Code Snippet
https://github.com/sherlock-audit/2023-12-arcadia/blob/de7289bebb3729505a2462aa044b3960d8926d78/lending-v2/src/LendingPool.sol#L1141-L1161
https://github.com/sherlock-audit/2023-12-arcadia/blob/de7289bebb3729505a2462aa044b3960d8926d78/lending-v2/src/LendingPool.sol#L1195-L1197
Tool used
Manual Review
Recommendation
Add checks in _calculateRewards such that if sum of initiation + termination > minimumMargin decrease them proportionally. Or always have maxRewards = minimumMargin/2 but that will defeat the purpose of initiation & termination rewards.
Duplicate of #1