Closed sherlock-admin4 closed 1 month ago
sunbreak1211
commented
1 month ago Acquiring a large amount of MKR through leverage is not a smart contract problem. Governane is assumed to set parameters while having such social/governance-related risks in mind.
Audittens
commented
1 month ago Escalate
Acquiring a large amount of MKR through leverage is not a smart contract problem.
While acquiring a large amount of any token through leverage is indeed not a smart contract problem (as it is stated in the issue, in common cases of leverage, the user only gains these extra leveraged tokens "on paper" and therefore is unable to use them in any way), the current issue focuses not on the acquiring of a large amount of MKR, but on its unfair utilization which becomes possible exclusively due to the functionality of LockstakeEngine and therefore it is a smart-contract problem.
Governane is assumed to set parameters while having such social/governance-related risks in mind.
While in theory the issue could be solved by setting Ilk.mat to infinitely large value, this argument cannot be applied here due to the following reasons:
Ilk.mat is limited by 1000% upperbound in the LockstakeInit.initLockstake() function. Even with theoretical maximum Ilk.mat = 1000%, the max leverage will be equal to $LEV = \frac{1}{1 - \frac{1}{10}} = \frac{10}{9}$. Using this $LEV$ value in two described impacts results in:
In the lockstake's README there is a special section devoted to the rationale for choosing a specific Ilk.mat value. In this section only the following risks are taken into account for justification of choosing Ilk.mat value:
Combining it with the fact that none of the previous audits raised the issue about possibility to unfairly utilize leveraged MKR, this risk cannot be considered known. Without having this risk in mind, Ilk.mat will never be set to unreasonably large value of 1000% (which still has problems as stated above), and instead will be set to much lower value.
While the first two risks above give the exact lower bound Ilk.mat > 133%, the third risk is more subjective and therefore the final value of Ilk.mat can vary. However, in tests Ilk.mat is set to 300% which is significantly larger than lower bound and has sufficient buffer for the third risk. Therefore the final value is likely supposed to be in range [133%; 300%].
The following table shows the strength of the impacts based by Ilk.mat value (the last column is calculated considering 137,551 MKR is used for voting as of August, 15):
Ilk.mat |
max LEV | % of additional reward from farming | % of MKR needed to become a chief | Cost of becoming a chief, USD |
|---|---|---|---|---|
| 133% (lower bound) | 4 | 300% | 12.5% | 35M |
| 150% | 3 | 200% | ~16.67% | ~46.67M |
| 200% | 2 | 100% | 25% | 70M |
| 300% (value used in tests) | 1.5 | 50% | ~33.33% | ~93.33M |
| 1000% (theoretical maximum) | ~1.11 | ~11.11% | 45% | 126M |
As can be seen from the table, any possible value of Ilk.mat leads to large enough percentage of additional rewards and therefore essential losses for regular users, which is already strong enough impact to be classified as at least Medium according to the contest rules.
At the same time, all reasonable values of Ilk.mat allow to significantly reduce amount of MKR needed to become a chief, which makes impact the same as in the confirmed issue #64 (impact stated in the current issue #114 "user being able to execute any spell they would like to" = impact stated in the confirmed issue #64 "attacker gains full control of the system through hat election"), but with much lower cost and time required for the attack (93 millions USD considering 137,551 MKR is used for voting as of August, 15 and no time instead of several billions of USD and 1 year of calculating hashes as shown in the EIP-3607). This impact additionally enhances severity of the issue.
sherlock-admin3
commented
1 month ago Escalate
Acquiring a large amount of MKR through leverage is not a smart contract problem.
While acquiring a large amount of any token through leverage is indeed not a smart contract problem (as it is stated in the issue,
in common cases of leverage, the user only gains these extra leveraged tokens "on paper" and therefore is unable to use them in any way), the current issue focuses not on the acquiring of a large amount of MKR, but on its unfair utilization which becomes possible exclusively due to the functionality of LockstakeEngine and therefore it is a smart-contract problem.Governane is assumed to set parameters while having such social/governance-related risks in mind.
While in theory the issue could be solved by setting
Ilk.matto infinitely large value, this argument cannot be applied here due to the following reasons:
Ilk.matis limited by 1000% upperbound in the LockstakeInit.initLockstake() function. Even with theoretical maximumIlk.mat = 1000%, the max leverage will be equal to $LEV = \frac{1}{1 - \frac{1}{10}} = \frac{10}{9}$. Using this $LEV$ value in two described impacts results in:
- reducing amount of MKR needed to become a majority holder of voting power from 50% to 45%.
- receiving farming rewards that are ~11.11% bigger at the expense of regular users, that still leads to their significant loss of funds.
In the lockstake's README there is a special section devoted to the rationale for choosing a specific
Ilk.matvalue. In this section only the following risks are taken into account for justification of choosingIlk.matvalue:
- ensuring enough MKR is left;
- preventing incentives for self-liquidation;
- possible market fluctuations.
Combining it with the fact that none of the previous audits raised the issue about possibility to unfairly utilize leveraged MKR, this risk cannot be considered known. Without having this risk in mind,
Ilk.matwill never be set to unreasonably large value of 1000% (which still has problems as stated above), and instead will be set to much lower value.Summary
While the first two risks above give the exact lower bound
Ilk.mat > 133%, the third risk is more subjective and therefore the final value ofIlk.matcan vary. However, in testsIlk.matis set to 300% which is significantly larger than lower bound and has sufficient buffer for the third risk. Therefore the final value is likely supposed to be in range[133%; 300%].The following table shows the strength of the impacts based by
Ilk.matvalue (the last column is calculated considering 137,551 MKR is used for voting as of August, 15):
Ilk.matmax LEV % of additional reward from farming % of MKR needed to become a chief Cost of becoming a chief, USD 133% (lower bound) 4 300% 12.5% 35M 150% 3 200% ~16.67% ~46.67M 200% 2 100% 25% 70M 300% (value used in tests) 1.5 50% ~33.33% ~93.33M 1000% (theoretical maximum) ~1.11 ~11.11% 45% 126M As can be seen from the table, any possible value of
Ilk.matleads to large enough percentage of additional rewards and therefore essential losses for regular users, which is already strong enough impact to be classified as at least Medium according to the contest rules.At the same time, all reasonable values of
Ilk.matallow to significantly reduce amount of MKR needed to become a chief, which makes impact the same as in the confirmed issue #64 (impact stated in the current issue #114 "user being able to execute any spell they would like to" = impact stated in the confirmed issue #64 "attacker gains full control of the system through hat election"), but with much lower cost and time required for the attack (93 millions USD considering 137,551 MKR is used for voting as of August, 15 and no time instead of several billions of USD and 1 year of calculating hashes as shown in the EIP-3607). This impact additionally enhances severity of the issue.
The escalation could not be created because you are not exceeding the escalation threshold.
You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.
Barichek
commented
1 month ago Escalate
Acquiring a large amount of MKR through leverage is not a smart contract problem.
While acquiring a large amount of any token through leverage is indeed not a smart contract problem (as it is stated in the issue, in common cases of leverage, the user only gains these extra leveraged tokens "on paper" and therefore is unable to use them in any way), the current issue focuses not on the acquiring of a large amount of MKR, but on its unfair utilization which becomes possible exclusively due to the functionality of LockstakeEngine and therefore it is a smart-contract problem.
Governane is assumed to set parameters while having such social/governance-related risks in mind.
While in theory the issue could be solved by setting Ilk.mat to infinitely large value, this argument cannot be applied here due to the following reasons:
Ilk.mat is limited by 1000% upperbound in the LockstakeInit.initLockstake() function. Even with theoretical maximum Ilk.mat = 1000%, the max leverage will be equal to $LEV = \frac{1}{1 - \frac{1}{10}} = \frac{10}{9}$. Using this $LEV$ value in two described impacts results in:
In the lockstake's README there is a special section devoted to the rationale for choosing a specific Ilk.mat value. In this section only the following risks are taken into account for justification of choosing Ilk.mat value:
Combining it with the fact that none of the previous audits raised the issue about possibility to unfairly utilize leveraged MKR, this risk cannot be considered known. Without having this risk in mind, Ilk.mat will never be set to unreasonably large value of 1000% (which still has problems as stated above), and instead will be set to much lower value.
While the first two risks above give the exact lower bound Ilk.mat > 133%, the third risk is more subjective and therefore the final value of Ilk.mat can vary. However, in tests Ilk.mat is set to 300% which is significantly larger than lower bound and has sufficient buffer for the third risk. Therefore the final value is likely supposed to be in range [133%; 300%].
The following table shows the strength of the impacts based by Ilk.mat value (the last column is calculated considering 137,551 MKR is used for voting as of August, 15):
Ilk.mat |
max LEV | % of additional reward from farming | % of MKR needed to become a chief | Cost of becoming a chief, USD |
|---|---|---|---|---|
| 133% (lower bound) | 4 | 300% | 12.5% | 35M |
| 150% | 3 | 200% | ~16.67% | ~46.67M |
| 200% | 2 | 100% | 25% | 70M |
| 300% (value used in tests) | 1.5 | 50% | ~33.33% | ~93.33M |
| 1000% (theoretical maximum) | ~1.11 | ~11.11% | 45% | 126M |
As can be seen from the table, any possible value of Ilk.mat leads to large enough percentage of additional rewards and therefore essential losses for regular users, which is already strong enough impact to be classified as at least Medium according to the contest rules.
At the same time, all reasonable values of Ilk.mat allow to significantly reduce amount of MKR needed to become a chief, which makes impact the same as in the confirmed issue #64 (impact stated in the current issue #114 "user being able to execute any spell they would like to" = impact stated in the confirmed issue #64 "attacker gains full control of the system through hat election"), but with much lower cost and time required for the attack (93 millions USD considering 137,551 MKR is used for voting as of August, 15 and no time instead of several billions of USD and 1 year of calculating hashes as shown in the EIP-3607). This impact additionally enhances severity of the issue.
sherlock-admin3
commented
1 month ago Escalate
Acquiring a large amount of MKR through leverage is not a smart contract problem.
While acquiring a large amount of any token through leverage is indeed not a smart contract problem (as it is stated in the issue,
in common cases of leverage, the user only gains these extra leveraged tokens "on paper" and therefore is unable to use them in any way), the current issue focuses not on the acquiring of a large amount of MKR, but on its unfair utilization which becomes possible exclusively due to the functionality of LockstakeEngine and therefore it is a smart-contract problem.Governane is assumed to set parameters while having such social/governance-related risks in mind.
While in theory the issue could be solved by setting
Ilk.matto infinitely large value, this argument cannot be applied here due to the following reasons:
Ilk.matis limited by 1000% upperbound in the LockstakeInit.initLockstake() function. Even with theoretical maximumIlk.mat = 1000%, the max leverage will be equal to $LEV = \frac{1}{1 - \frac{1}{10}} = \frac{10}{9}$. Using this $LEV$ value in two described impacts results in:
- reducing amount of MKR needed to become a majority holder of voting power from 50% to 45%.
- receiving farming rewards that are ~11.11% bigger at the expense of regular users, that still leads to their significant loss of funds.
In the lockstake's README there is a special section devoted to the rationale for choosing a specific
Ilk.matvalue. In this section only the following risks are taken into account for justification of choosingIlk.matvalue:
- ensuring enough MKR is left;
- preventing incentives for self-liquidation;
- possible market fluctuations.
Combining it with the fact that none of the previous audits raised the issue about possibility to unfairly utilize leveraged MKR, this risk cannot be considered known. Without having this risk in mind,
Ilk.matwill never be set to unreasonably large value of 1000% (which still has problems as stated above), and instead will be set to much lower value.Summary
While the first two risks above give the exact lower bound
Ilk.mat > 133%, the third risk is more subjective and therefore the final value ofIlk.matcan vary. However, in testsIlk.matis set to 300% which is significantly larger than lower bound and has sufficient buffer for the third risk. Therefore the final value is likely supposed to be in range[133%; 300%].The following table shows the strength of the impacts based by
Ilk.matvalue (the last column is calculated considering 137,551 MKR is used for voting as of August, 15):
Ilk.matmax LEV % of additional reward from farming % of MKR needed to become a chief Cost of becoming a chief, USD 133% (lower bound) 4 300% 12.5% 35M 150% 3 200% ~16.67% ~46.67M 200% 2 100% 25% 70M 300% (value used in tests) 1.5 50% ~33.33% ~93.33M 1000% (theoretical maximum) ~1.11 ~11.11% 45% 126M As can be seen from the table, any possible value of
Ilk.matleads to large enough percentage of additional rewards and therefore essential losses for regular users, which is already strong enough impact to be classified as at least Medium according to the contest rules.At the same time, all reasonable values of
Ilk.matallow to significantly reduce amount of MKR needed to become a chief, which makes impact the same as in the confirmed issue #64 (impact stated in the current issue #114 "user being able to execute any spell they would like to" = impact stated in the confirmed issue #64 "attacker gains full control of the system through hat election"), but with much lower cost and time required for the attack (93 millions USD considering 137,551 MKR is used for voting as of August, 15 and no time instead of several billions of USD and 1 year of calculating hashes as shown in the EIP-3607). This impact additionally enhances severity of the issue.
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.
sunbreak1211
commented
4 weeks ago The risk parameters will be set by Maker governance using the Maker risk stewards (currently BA Labs). As highlighted in the contest readme (first item in Governance section): "Governance configurations are assumed to be set with extreme care."
The submission states "Without having this risk in mind..", but given the above it should be assumed to be in mind. Therefore it is out of scope of the competition to assume that governance will set values recklessly and allow governance attacks. Obviously when lending against your governance token such considerations are taken into account.
As an example of why risk considerations in configuration are a good thing to be highlighted but are not a smart contract audit vulnerability see issue 8.3 "Governance Token as Collateral" in https://github.com/makerdao/lockstake/blob/dev/audit/20240730-ChainSecurity_MakerDAO_Lockstake_audit.pdf which is categorized as an informational note.
As a side note, from having only the code in hand it is hard to assess the effects of the lockstake engine on the possibility of governance attacks. There are for example claims that the lockstake engine will actually reduce the option of governance attacks since it will result in less borrowable MKR on other platforms (in the discussed issue MKR is not borrowable so you need to buy it during the looping, which has a heavy cost). The point is that such risk-related arguments are to be weighed carefully by the protocol's risk steward and can not assumed known from auditing the code.
Audittens
commented
4 weeks ago Thank you for providing additional feedback. However, I still think that there is some misunderstanding of the current issue.
As an example of why risk considerations in configuration are a good thing to be highlighted but are not a smart contract audit vulnerability see issue 8.3 "Governance Token as Collateral" in https://github.com/makerdao/lockstake/blob/dev/audit/20240730-ChainSecurity_MakerDAO_Lockstake_audit.pdf which is categorized as an informational note.
The issue 8.3 shows a general warning about borrowing stablecoin against the governance token. It talks about tumultuous situations when a stablecoin loses its peg which leads to a drop of the governance token price. Such concern exists for any pair "stablecoin" - "governance token", and indeed is informational due to the extremely low likelihood of happening in practice with the correct configuration. The same issue is possible even in the simplified version of the LockstakeEngine that doesn't have the functionality to either vote through VoteDelegate, or use farms to earn rewards. I.e. just creating an ilk with a governance token accepted as collateral introduces the risk described in the 8.3 issue. Note that in such a configuration, it's also not possible to exploit leveraging, because any acquired MKR would be just locked in the ilk, and only the NST from the last draw() could be used.
Contrary to 8.3, current issue #114 shows another impact, that becomes possible exclusively due to the excessive functionality of LockstakeEngine. Therefore current issue #114 shows LockstakeEngine smart-contract problem, which can be fixed on the smart-contract level, contrary to the well-known and more general&theoretical risk of allowing borrows against governance token, which cannot be fixed on the smart-contract level.
Therefore it is out of scope of the competition to assume that governance will set values recklessly
As I shown in the escalation above, due to the upperbound of Ilk.mat by 1000%, governance cannot configure LockstakeEngine in such a way that mitigates the described problem. Even setting Ilk.mat to the maximum value of 1000% would allow an attacker to gain an extra 11.11% of rewards at the expense of regular users, which cannot be considered negligible losses for them.
The only way to mitigate the described problem by governance configuration is to set Ilk.mat to extremely large values (e.g. with Ilk.mat = 20000% the extra rewards from farms will still be about 0.5%, which in theory can be considered affordable). But this is not only impossible due to upperbound of Ilk.mat by 1000%, but also doesn't make any sense from the project perspective, because with such an extremely large Ilk.mat no user would like to take even a minimum by Ilk.dust borrow of ~5000 NST against his MKR that costs ~1'000'000$. If it was the view of the project to set Ilk.mat to such large value, then it would just not implement the functionality of taking borrows against MKR.
it will result in less borrowable MKR on other platforms (in the discussed issue MKR is not borrowable so you need to buy it during the looping, which has a heavy cost)
Firstly, it's supposed to be a big Uniswap-V2 pool that has a large liquidity provided by pause proxy. Currently Uniswap-V2 pool for DAI-MKR pair holds about 73 million USD of both DAI and MKR. Also it's possible to buy MKR not all at once, but in several operations, between which some other users or MEV-bots will bring additional MKR to the pool.
Except for Uniswap-V2 pool, there are a lot of MKR available on other platforms (e.g. DEXs, CEXs, etc). While I agree that the amount of available MKR on other platforms could significantly decrease after sufficient time passed since LockstakeEngine launch, nothing prevents the attacker from performing the attack immediately after the LockstakeEngine launch. In such a case, a lot of MKR will still be available on other platforms, while the amount of MKR locked in chief will remain approximately the same, because some time is required for regular users to lock MKR in the LockstakeEngine.
So it should not be a problem to buy a necessary amount of MKR at a good price, especially for the impact of gaining extra rewards from farms, which requires much less MKR to buy.
WangSecurity
commented
4 weeks ago @Audittens but I believe your above argument still doesn't change the fact that this issue is possible due to specific governance configurations. Moreover, the README states that the governance will choose the parameters with extreme care and the admin limitations question has a "No" answer. As I understand, the issue is not possible with all possible admin values and only with a specific set of them. Hence, this issue should remain invalid, since it speculates on governance setting specific configs. Planning to reject the escalation.
Audittens
commented
4 weeks ago @WangSecurity no, as I stated above the governance cannot set the Ilk.mat parameter bigger than 1000% due to corresponding require() statement inside initLockstake() function. All possible values of Ilk.mat that admin can set (that range from 100% to 1000%) lead to significant enough loss of funds. Please refer to my escalation comment above, where I described exact amount of fund losses for different (including extreme) values of Ilk.mat configuration.
Moreover, the README states that the governance will choose the parameters with extreme care and the admin limitations question has a "No" answer.
I answered this statement in my comment above while answering wider statement about governance configuration being out of scope ("Therefore it is out of scope of the competition to assume that governance will set values recklessly").
WangSecurity
commented
3 weeks ago To clarify, this issue is dependant only on Ilk.mat value and other governance configuration values have no effect on this issue, i.e. they can't mitigate it, correct?
Could you also make a coded POC showing how the user gets a larger voting power?
Audittens
commented
3 weeks ago While coding POC and checking other governance configuration values, I realized that Ilk.line (the maximum amount of DAI that can be borrowed against MKR, which has order of magnitude $\sim[10^6; 10^7]$ DAI) will prevent the attacker from getting MKR that is necessary to become a chief. So unfortunately the first shown impact is impossible to utilize. I apologize for not noticing this limitation earlier, and I agree that without this impact, the severity of the issue cannot be considered High.
However, the second impact is still possible, because it requires much less MKR. As I stated in the penultimate paragraph of "Summary" section of my escalation comment, this impact itself is strong enough to be classified as Medium, because it leads to significant enough loss of users funds.
Below you can find a coded POC that inherits the setup from LockstakeEngine.t.sol (which in particular has Ilk.mat = 300%). This POC shows how an attacker that has the same amount of MKR as a regular user, can utilize leverage to unfairly lock in farm $LEV = 1.5$ times more lsMKR which results in net profit of 20% for the attacker and 20% loses for the regular user.
Note that while the $LEV$ coefficient depends only on the Ilk.mat configuration (you can see how this coefficient vary in the table from the escalation comment above), in real setup (when there will be several other regular users and potentially several attackers), the net profit and loses vary based on how much percentage of MKR from farm.totalSupply() the attacker (or several attackers) has:
In practice, the exact values of net profit and users loses will be somewhere in between of $0$ and $(LEV - 1)$.
test/LeverageTest.sol file inside lockstake directory.
// SPDX-License-Identifier: AGPL-3.0-or-later
pragma solidity ^0.8.21;
import "test/LockstakeEngine.t.sol";
import "forge-std/console.sol";
interface UniswapV2Pair {
function getReserves() external view returns (uint112 _reserve0, uint112 _reserve1, uint32 _blockTimestampLast);
function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external;
}
library UniswapV2Library {
function _getAmountOut(uint256 amtIn, uint256 reserveIn, uint256 reserveOut) internal pure returns (uint256 amtOut) {
uint256 _amtInFee = amtIn * 997;
amtOut = _amtInFee * reserveOut / (reserveIn * 1000 + _amtInFee);
}
}
contract LeverageTest is LockstakeEngineTest {
address regularUser;
address attacker;
address regularUserUrn;
address attackerUrn;
address constant uniswapV2MkrDai = 0x517F9dD285e75b599234F7221227339478d0FcC8; // address from main-net
function openUrnFrom(address user) internal returns(address urn) {
vm.startPrank(user);
urn = engine.open(0);
vm.stopPrank();
}
function customSetUp() internal {
regularUser = makeAddr("regularUser");
attacker = makeAddr("attacker");
vm.startPrank(pauseProxy);
// both regular user and attacker initially has the same amount of 100 MKR ~ 200'000 DAI
mkr.mint(regularUser, 100 ether);
mkr.mint(attacker, 100 ether);
// setting realistic price of MKR
(uint112 reservesDAI, uint112 reservesMKR, ) = UniswapV2Pair(uniswapV2MkrDai).getReserves();
uint price = uint(reservesDAI) * 10 ** 18 / reservesMKR;
console.log("1e18 MKR = %s DAI", toE18Precision(price));
vm.store(address(pip), bytes32(uint256(1)), bytes32(price));
dss.spotter.poke(ilk);
vm.stopPrank();
// both users opens their urns to use LockstakeEngine
regularUserUrn = openUrnFrom(regularUser);
attackerUrn = openUrnFrom(attacker);
// approvals for exchanging NST to DAI
vm.startPrank(attacker);
nst.approve(address(nstJoin), type(uint256).max);
dss.vat.hope(address(dss.daiJoin));
vm.stopPrank();
}
function lockAllMkrFrom(address user, address urn) internal {
uint amount = mkr.balanceOf(user);
vm.startPrank(user);
mkr.approve(address(engine), amount);
engine.lock(urn, amount, 0);
vm.stopPrank();
}
function selectFarmFrom(address user, address urn, address farm) internal {
vm.startPrank(user);
engine.selectFarm(urn, farm, 0);
vm.stopPrank();
}
function nstToDai(address user, uint256 wad) internal {
nstJoin.join(user, wad);
dss.daiJoin.exit(user, wad);
}
function testLeverage() public {
customSetUp();
lockAllMkrFrom(regularUser, regularUserUrn);
lockAllMkrFrom(attacker, attackerUrn);
// attacker exploits leverage to get bigger rewards from farm at the expense of regular user
for (uint i = 1; i <= 4; ++i) {
vm.startPrank(attacker);
uint currentInk = _ink(ilk, attackerUrn);
uint maxTab = currentInk * _spot(ilk);
uint maxArt = maxTab / _rate(ilk);
uint maxWadToDraw = maxArt - _art(ilk, attackerUrn);
console.log("Iteration %d, drawing %s NST", i, toE18Precision(maxWadToDraw));
engine.draw(attackerUrn, attacker, maxWadToDraw);
nstToDai(attacker, maxWadToDraw); // exchanging NST to DAI
// exchanging DAI to MKR on UniswapV2
(uint112 reservesDAI, uint112 reservesMKR, ) = UniswapV2Pair(uniswapV2MkrDai).getReserves();
console.log("Price of MKR based on reserves on the iteration %d: 1e18 MKR = %s DAI", i, toE18Precision(uint(reservesDAI) * 1e18 / reservesMKR));
uint maxMKRToBuy = UniswapV2Library._getAmountOut(maxWadToDraw, reservesDAI, reservesMKR);
console.log("Iteration %d, buying %s MKR", i, toE18Precision(maxMKRToBuy));
dss.dai.transfer(address(uniswapV2MkrDai), maxWadToDraw);
UniswapV2Pair(uniswapV2MkrDai).swap(0, maxMKRToBuy, attacker, new bytes(0));
assertEq(mkr.balanceOf(attacker), maxMKRToBuy);
// locking MKR into LSE again
lockAllMkrFrom(attacker, attackerUrn);
vm.stopPrank();
console.log("Total amount of locked MKR after iteration %d: %s", i, toE18Precision(lsmkr.balanceOf(attackerUrn)));
console.log("");
}
// attacker now has ~1.5 times more lsMKR that is used for earning rewards
assertApproxEqRel(150 * 10 ** 18, lsmkr.balanceOf(attackerUrn), 0.05 * 10 ** 18);
// both regular user and attacker select the same farm to get rewards
selectFarmFrom(regularUser, regularUserUrn, address(farm));
selectFarmFrom(attacker, attackerUrn, address(farm));
console.log("reg user staked balance: %s", toE18Precision(farm.balanceOf(regularUserUrn)));
console.log("attacker staked balance: %s", toE18Precision(farm.balanceOf(attackerUrn)));
// both regular user and attacker gets proportionally distributed rewards
uint totalRewards = 100 ether;
farm.setReward(address(regularUserUrn), farm.balanceOf(regularUserUrn) * totalRewards / farm.totalSupply());
farm.setReward(address(attackerUrn), farm.balanceOf(attackerUrn) * totalRewards / farm.totalSupply());
vm.startPrank(attacker);
engine.getReward(attackerUrn, address(farm), attacker);
vm.stopPrank();
vm.startPrank(regularUser);
engine.getReward(regularUserUrn, address(farm), regularUser);
vm.stopPrank();
console.log("reg user rewards: %s", toE18Precision(rTok.balanceOf(regularUser)));
console.log("attacker rewards: %s", toE18Precision(rTok.balanceOf(attacker)));
// attacker receives ~1.5 times more rewards than regular user, despite having the same MKR amount for locking
assertApproxEqRel(rTok.balanceOf(attacker), 3 * rTok.balanceOf(regularUser) / 2, 0.05 * 10 ** 18);
// instead of receiving the same reward of 50 ether, attacker gains 20% more, while the regular user loses 20% of his rewards funds
assertApproxEqRel(rTok.balanceOf(regularUser), 40 ether, 0.05 * 10 ** 18);
assertApproxEqRel(rTok.balanceOf(attacker), 60 ether, 0.05 * 10 ** 18);
}
function toE18Precision(uint x) internal pure returns(string memory) {
uint integerPart = x / 1e18;
uint length = integerPart == 0 ? 1 : 0;
for (uint y = integerPart; y > 0; y /= 10) {
++length;
}
uint realPart = (x % 1e18) / 1e14;
bytes memory b = new bytes(length + 1 + 4 + 3);
for (uint i = 0; i < length; ++i) {
b[length - 1 - i] = bytes1(bytes1(uint8(48 + integerPart % 10)));
integerPart /= 10;
}
b[length] = bytes1(uint8(46)); // .
for (uint i = 0; i < 4; ++i) {
b[length + 4 - i] = bytes1(bytes1(uint8(48 + realPart % 10)));
realPart /= 10;
}
b[length + 5] = bytes1(uint8(101)); // e
b[length + 6] = bytes1(uint8(49)); // 1
b[length + 7] = bytes1(uint8(56)); // 8
return string(b);
}
}dss-flappers/lib/dss-test/* folder into lockstake/lib/export ETH_RPC_URL=<Mainnet URL> (I used my link from https://app.infura.io as a <Mainnet URL>)forge test --match-test testLeverage -vv from the lockstake directory$ forge test --match-test testLeverage -vv
[⠒] Compiling...
[⠘] Compiling 1 files with Solc 0.8.21
[⠊] Solc 0.8.21 finished in 13.72s
Compiler run successful!
Ran 1 test for test/LeverageTest.sol:LeverageTest
[PASS] testLeverage() (gas: 2260503)
Logs:
$ forge test --match-test testLeverage -vv
[⠒] Compiling...
[⠔] Compiling 1 files with Solc 0.8.21
[⠒] Solc 0.8.21 finished in 14.36s
Compiler run successful!
Ran 1 test for test/LeverageTest.sol:LeverageTest
[PASS] testLeverage() (gas: 2262418)
Logs:
1e18 MKR = 1973.2010e18 DAI
Iteration 1, drawing 65773.3672e18 NST
Price of MKR based on reserves on the iteration 1: 1e18 MKR = 1973.2010e18 DAI
Iteration 1, buying 33.2040e18 MKR
Total amount of locked MKR after iteration 1: 133.2040e18
Iteration 2, drawing 21839.4395e18 NST
Price of MKR based on reserves on the iteration 2: 1e18 MKR = 1976.6849e18 DAI
Iteration 2, buying 11.0121e18 MKR
Total amount of locked MKR after iteration 2: 144.2162e18
Iteration 3, drawing 7243.0642e18 NST
Price of MKR based on reserves on the iteration 3: 1e18 MKR = 1977.8424e18 DAI
Iteration 3, buying 3.6507e18 MKR
Total amount of locked MKR after iteration 3: 147.8669e18
Iteration 4, drawing 2401.2301e18 NST
Price of MKR based on reserves on the iteration 4: 1e18 MKR = 1978.2264e18 DAI
Iteration 4, buying 1.2101e18 MKR
Total amount of locked MKR after iteration 4: 149.0771e18
reg user staked balance: 100.0000e18
attacker staked balance: 149.0771e18
reg user rewards: 40.1482e18
attacker rewards: 59.8517e18
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 27.93s (7.30s CPU time)
Ran 1 test suite in 27.94s (27.93s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
oldchili
commented
3 weeks ago Why is using leverage for buying more MKR and locking it for farm rewards considered an attack? The user who does that pays the system stability fees for their additional loan, drives MKR price up by buying it in the market and locks more MKR behind an exit fee. All of these are net profit for the system, so this is more of a feature, and definitely not a bug.
Also, there is no guarantee or definition of "fairness" with regards to farm rewards, any way to get MKR for farm usage should be legitimate. So it shouldn't be assumed that using leverage causes other users to lose anything.
Lastly, it could maybe be argued that the farming rewards might be so big that the system would ultimately lose funds to the user, but that again becomes a discussion about configurations (plus a philosophic discussion about short-term incentives).
Audittens
commented
3 weeks ago Thank you, oldchili, for the reasonable questions.
Why is using leverage for buying more MKR and locking it for farm rewards considered an attack?
To understand why, let's consider an extreme example where it's possible to utilize enormous leverage, e.g x100 of the initial funds. Starting with a negligible amount of 0.01 MKR, the attacker can lock a substantial amount of 1 MKR into the farm. After a reasonable amount of time, the rewards will significantly exceed the initial amount of the initial attacker's funds. Therefore, the attacker may not worry about paying any fees, because even if he will not withdraw his MKR, or will be completely liquidated in the future, he still unfairly earns much more from the rewards. Of course, this is an extreme example with unrealistic numbers, but it clearly shows from the theoretical point of view why using leverage to buy more MKR and lock it for farm rewards should be considered an attack.
any way to get MKR for farm usage should be legitimate
If a way to obtain MKR for farm usage arises due to a bug in LSE (which is the case as I demonstrated in my response to the previous question), this way should not be considered legitimate. To understand why, let's consider another example. Suppose LSE contains a bug that allows an attacker to use for his own farm MKR of other users who haven't selected their farm. In this scenario, the attacker could obtain MKR for farm usage as a result of the bug, and obviously this way of obtaining MKR should not be considered legitimate.
oldchili
commented
3 weeks ago For the first part of your comment ("extreme example with unrealistic numbers" as you said) see my last paragraph in the previous comment. Of course some weird configurations can make the farm rewards profitable vs the borrowing fee + the cost of buying MKR + the exit fee, but if so it's a configuration issue (which is again, out of scope).
For your second part, no bug was demonstrated. Hypothetically, if a vulnerability that would allow ppl to steal or get MKR completely freely would have existed it would be a problem on it's own. Not the fact that the MKR could be used for farming later.
Anyway, I think we said enough from the protocol side here.
Audittens
commented
3 weeks ago While looking more carefully on the configuration details I realized that in fact Ilk.line is not an obstacle for the first impact about getting large voting power. As stated in the contest rules, cfg.maxLine (which is the amount of DAI that eventually will be available for borrowing) is defined to be 100M dai for the contest, which is enough to significantly increase the initial voting power.
And since Ilk.line is not an obstacle anymore, any other governance configuration (except Ilk.mat which is limited by 1000%) cannot prevent the attack. I'm sorry for this confusion once again.
Here's the coded POC that shows how the attacker can get a larger voting power.
Below you can find a coded POC that inherits the setup from LockstakeEngine.t.sol (which in particular has Ilk.mat = 300%). This POC shows how an attacker who initially has 80000 MKR can utilize leverage to lock ~1.5 * 80000 MKR in his VoteDelegate and therefore "beat" a current chief.hat() that currently has a voting power of ~113000 MKR.
Due to the nature of MCD_IAM_AUTO_LINE not all cfg.maxLine = 100M DAI will be available for borrowing immediately after the LockstakeEngine launch. Instead, the cfg.gap will be available initially, and the exact limit Ilk.line can be increased once in cfg.ttl seconds from the current total debt d to the d + cfg.gap. For POC the exact values of cfg.gap and cfg.ttl were chosen in the same as way as for the tests: cfg.gap = 10M DAI (10% of the cfg.maxLine), cfg.ttl = 1 days.
In such setup starting from the initial 80000 MKR = 160M DAI, attacker can get additional voting power of 40000 MKR = 80M DAI by taking debt in portions of $D$ ~10M DAI (one portion per day):
engine.draw(D) to borrow $D$ DAI against his locked MKR.web3Community) that monitor UniswapV2 pool, sell their MKR due to the attractable DAI price, and therefore return back price of MKR to the reasonable amount. Note, that in this POC, UniswapV2 pool is used for the sake of simplicity. In practice attacker can use several distinct platforms (DEXs, CEXs) to sell $D$ DAI in the best possible price. It should be possible because:
web3Community holds large enough amount of MKR. For this, I used estimation mkr.totalSupply() - mkr.balanceOf(attacker) - mkr.balanceOf(chief) - mkr.balanceOf(pauseProxy) ~ 1'000'000 - 80'000 - 140'000 - 50'000 = 730'000 MKR.web3Community there'll be enough parties that are incentivized to keep pools up to date or just earn profit on the price movements after attacker's actions.MCD_IAM_AUTO_LINE.exec(ilk) and increasses Ilk.line amount by another 10M DAI.Note, that the exact values of cfg.gap and cfg.ttl influence only the total duration of the attack, but cannot prevent it.
test/ChiefTest.sol file inside lockstake directory:dss-flappers/lib/dss-test/* folder into lockstake/lib/.export ETH_RPC_URL=<Mainnet URL> (I used my link from https://app.infura.io as a ETH_RPC_URL).forge test --match-test testChief -vv from the lockstake directory.[⠒] Compiling...
[⠔] Compiling 1 files with Solc 0.8.21
[⠒] Solc 0.8.21 finished in 14.41s
Compiler run successful!
Ran 1 test for test/ChiefTest.sol:ChiefTest
[PASS] testChief() (gas: 5561373)
Logs:
1e18 MKR = 2038.9879e18 DAI
Attacker initially has 80000.0000e18 MKR
Chief hat has 112922.7313e18 MKR
web3Community has 650000.0000e18 MKR
Iteration 1, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 1: 1e18 MKR = 2038.9879e18 DAI
Bought 4641.1333e18 MKR on the iteration 1
After iteration 1 attacker has 84641.1333e18 of locked MKR
After iteration 1 Ilk.line = 19800000.0000e18
Iteration 2, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 2: 1e18 MKR = 2040.5170e18 DAI
Bought 4637.7648e18 MKR on the iteration 2
After iteration 2 attacker has 89278.8982e18 of locked MKR
After iteration 2 Ilk.line = 29608470.8588e18
Iteration 3, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 3: 1e18 MKR = 2042.0461e18 DAI
Bought 4634.4011e18 MKR on the iteration 3
After iteration 3 attacker has 93913.2993e18 of locked MKR
After iteration 3 Ilk.line = 39425419.8985e18
Iteration 4, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 4: 1e18 MKR = 2043.5752e18 DAI
Bought 4631.0421e18 MKR on the iteration 4
After iteration 4 attacker has 98544.3415e18 of locked MKR
After iteration 4 Ilk.line = 49250854.4473e18
Iteration 5, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 5: 1e18 MKR = 2045.1044e18 DAI
Bought 4627.6880e18 MKR on the iteration 5
After iteration 5 attacker has 103172.0295e18 of locked MKR
After iteration 5 Ilk.line = 59084781.8399e18
Iteration 6, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 6: 1e18 MKR = 2046.6336e18 DAI
Bought 4624.3386e18 MKR on the iteration 6
After iteration 6 attacker has 107796.3682e18 of locked MKR
After iteration 6 Ilk.line = 68927209.4172e18
Iteration 7, drawing 9800000.0000e18 NST
Price of MKR based on reserves on the iteration 7: 1e18 MKR = 2048.1629e18 DAI
Bought 4620.9940e18 MKR on the iteration 7
After iteration 7 attacker has 112417.3622e18 of locked MKR
After iteration 7 Ilk.line = 78778144.5267e18
Iteration 8, drawing 7372200.9985e18 NST
Price of MKR based on reserves on the iteration 8: 1e18 MKR = 2049.6922e18 DAI
Bought 3500.8427e18 MKR on the iteration 8
After iteration 8 attacker has 115918.2050e18 of locked MKR
After iteration 8 Ilk.line = 86209795.5205e18
Iteration 9, drawing 2442473.3591e18 NST
Price of MKR based on reserves on the iteration 9: 1e18 MKR = 2050.8515e18 DAI
Bought 1177.9002e18 MKR on the iteration 9
After iteration 9 attacker has 117096.1052e18 of locked MKR
After iteration 9 Ilk.line = 88718142.5960e18
Iteration 10, drawing 777602.4147e18 NST
Price of MKR based on reserves on the iteration 10: 1e18 MKR = 2051.2417e18 DAI
Bought 376.9852e18 MKR on the iteration 10
After iteration 10 attacker has 117473.0905e18 of locked MKR
After iteration 10 Ilk.line = 89563786.8755e18
As a result, attacker has x1.4684e18 time larger voting power than at the beginning
Chief.hat() has 112922.7313e18 votes
Attacker's voteDelegate has 117473.0905e18 votes
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 27.82s (7.45s CPU time)
Ran 1 test suite in 27.83s (27.82s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
P.S. While implementing POC, I realized that the initial amount of MKR needed for the attacker is bigger because it needs to collect not the half of the MKR that current chief.hat() has, but the same amount. Considering that current vote power of chief.hat() is ~113'000 MKR = 226M DAI (for the price of 1 MKR is ~2000$), the exact amount of initial attacker's funds needed to become a chief for Ilk.mat = 300% is ~75000 MKR = 150M$. The corresponding amount for other Ilk.mat values can be found in the table below:
WangSecurity
commented
3 weeks ago @Audittens thank you for this POC and such an extensive description. Could you also elaborate on the ratio between the cost of the attack and the funds lost? I see that the impact is that the attacker receives 3x more rewards, essentially stealing them from other voters, correct? But, the cost of such an attack is quite high. Maybe, I'm missing it inside the POC, but I assume the attack cost exceeds the loss of funds, but for how much? Is it 10%, 20%, 50% or even more larger than the loss of funds?
Audittens
commented
3 weeks ago As stated in the "Impact" section of my issue, there're two independent impacts (and therefore two different PoC):
Ilk.dust amount (several thousands of DAI) under his MKR of the same order of magnitude. Therefore for this impact, cost of the attack measures in thousands USD.I see that the impact is that the attacker receives 3x more rewards, essentially stealing them from other voters, correct? But, the cost of such an attack is quite high.
So answering your question, it's not correct, because it seems like you mixed up cost of the attack from the first impact, with the second impact about getting more rewards. As stated in the original issue, getting majority of voting power "leads to our user being able to execute any spell they would like to". In other words, it gives the attacker full control of the system.
Since the first impact is much stronger and only it requires high cost of the attack, in the following section I'll provide the comparison between cost of the attack of the first impact with profit from it (please tell me, if you need the corresponding analysis for the second impact).
The attack cost depends only on Ilk.mat configuration parameter and price of the MKR. For simplicity, let's assume that MKR have a fixed price of 2000 USD. Then the attack cost varies from 126M USD (for Ilk.mat = 200%) to 203M USD (for the extreme maximum value Ilk.mat = 1000%).
The profit from the attack doesn't depend on the attack cost. After getting a majority of voting power, attacker gains full control on the PauseProxy - contract, that can execute any spell. Having this control, the attacker for example can:
For this he need:
Only this is already 94M + 146M = 240M USD, which is more than the attack cost.
For this he need:
According the contest rules, the amount of NST locked in SNst can be assumed to be about 1B USD. Perfoming two actions above, attacker not only increases his profit by an additional up to 1B USD, but also now steals these funds from regular uses, making them bear 100% loses of their funds.
For this he need:
vat.slip() function and add to his virtual gem balance infinite balance of the specific collateral type. He can do it because PauseProxy is a ward of the vat contract.GemJoin.exit() function to obtain a collateral. This will steal all the collateral of other users.For example, only two ETH ilks have about 2.8B USD of locked WETH:
All other collaterals types can be stolen in the same way, which will result in ~100% loss of funds for protocol (it will not have any of the collateral left), and ~100% loss of funds for users (DAI will become worthless, because it loses its peg to the collateral).
As a conclusion, performing these actions will result in the profit of >= 240M + 1B + 1.1B + 1.7B = 4B USD (>= due to using in the calculations only two collateral types WETH-A and WETH-C) and 100% loss of funds for both protocol and users. Therefore the attack cost is significantly smaller than both profit for the attacker and loss of funds for the protocol/users.
Maybe, I'm missing it inside the POC
Your're not missing it, the current POC only shows how the attacker gets a large voting power required to become a chief.hat(). As it can be seen in the last commented out lines of the PoC, actual becoming to chief.hat() is not shown for the sake of simplicity, because the default setup LockstakeEngine.t.sol uses VoteDelegateMock and VoteDelegateFactoryMock instead of real VoteDelegate and VoteDelegateFactory contracts. If needed, please tell me and I'll replace these mocks by real contracts, and add the final steps of realizing the profit of the attack at the expense of corresponding protocol/users loses.
0xjuaan
commented
3 weeks ago What happened to this? :
While coding POC and checking other governance configuration values, I realized that Ilk.line (the maximum amount of DAI that can be borrowed against MKR, which has order of magnitude ∼ $[10^6,10^7]$ DAI) will prevent the attacker from getting MKR that is necessary to become a chief. So unfortunately the first shown impact is impossible to utilize. I apologize for not noticing this limitation earlier, and I agree that without this impact, the severity of the issue cannot be considered High.
Based on that comment from the submitter, becoming a chief is not possible.
So the only impact shown here is that the 'attacker' gets more rewards since they stake more MKR.
I don't see how this can be seen as an attack. They could also just get a leveraged loan from any lending protocol, and deposit those MKR tokens into Lockstake to earn higher rewards.
As the sponsor mentioned:
Also, there is no guarantee or definition of "fairness" with regards to farm rewards, any way to get MKR for farm usage should be legitimate. So it shouldn't be assumed that using leverage causes other users to lose anything.
The attacker simply risks their MKR (higher risk of being liquidated due to leverage), in return receiving more staking rewards due to having more MKR staked. This is intended functionality.
hildingr
commented
3 weeks ago I don't see how this can be seen as an attack. They could also just get a leveraged loan from any lending protocol, and deposit those MKR tokens into Lockstake to earn higher rewards.
This is true but the loan has to be undercollateralized. A user could do the same thing if they have access to undercollateralizde loans either on-chain or off-chain - they get leverage that they can then point at any incentive system or a governance system. The difference is just that Maker does not receive the fees for taking out the loan.
If this is a viable attack then so would the following be: A user takes out a loan either on-chain or off-chain that gives them 11% leverage, uses the funds to buy MKR and perform the "attack".
The above is obviously not a smart contract vulnerability since every coin based goverance/reward project is susceptible to it and the only solution is to move away from token based reward/governance systems. A lot has been written on the risk of coin based governance
Audittens
commented
3 weeks ago What happened to this? Based on that comment from the submitter, becoming a chief is not possible.
As I stated in my later comment, initially I underestimated cfg.maxLine value a lot, because I was confused by the wrong cfg.maxLine value in tests. In fact, for the contest it's clearly defined that cfg.maxLine = 100M (not 1M or 10M) and that's enough for the first impact about becoming a chief.hat(). I stated this in the same comment and provided a POC that proves that it's indeed possible to become a chief.hat() under cfg.maxLine = 100M configuration.
So the only impact shown here is that the 'attacker' gets more rewards since they stake more MKR. I don't see how this can be seen as an attack. They could also just get a leveraged loan from any lending protocol, and deposit those MKR tokens into Lockstake to earn higher rewards.
The majority of lending protocols onchain allow to take only overcollateralized debts, because with undercollateralized debt setup, protocol should somehow trust user, as otherwise he can just take a loan and never return it back. With a trusted setup it'll not be possible to easily take a huge loan, because it'll pose a huge risk for the protocol. And with overcollateralized debt setup (where you can take a huge loan against a huge collateral), you cannot get more MKR for use than you initially have. This is what is stated in my original issue: "In common cases of leverage, the user only gains these 2 extra MKR "on paper" and therefore is unable to use them in any way". For example, if you initially have 1 MKR deposited in some lending protocol, you can borrow 1000 USDC, exchange it to 0.5 MKR, deposit again, take another loan of 500 USDC, exchange it to 0.25 MKR, and so on. But at the end you have leveraged value "on paper" only: you indeed have 1.5 MKR locked in the lending protocol (more than you have initially), but you can use only 0.25 MKR from the last exchange operation. Therefore you cannot use standard leverage in other lending protocols to lock more MKR in the LockstakeEngine than you initially have.
Lastly, as I shown above, the main much stronger impact remains being about getting the majority of voting power and becoming a chief.hat(). Therefore I don't think there is much sense in continuing to discuss the second weaker impact, due to already long enough discussion. This is the reason why I elaborated only the first impact in my last comment.
If this is a viable attack then so would the following be: A user takes out a loan either on-chain or off-chain that gives them 11% leverage, uses the funds to buy MKR and perform the "attack".
As you correctly noticed above, only undercollateralized loans can help the attacker to increase the MKR funds. But for the first impact about getting the majority of voting power the attacker has to take a loan of tens of millions USD. In the trusted setup with undercollateralized debt no one will give to a noname attacker such a big loan. At the same time, in the current issue, attacker can take and use an additional loan of up to 100M USD in a trustless setup, which will always be possible.
hildingr
commented
3 weeks ago As you correctly noticed above, only undercollateralized loans can help the attacker to increase the MKR funds. But for the first impact about getting the majority of voting power the attacker has to take a loan of tens of millions USD. In the trusted setup with undercollateralized debt no one will give to a noname attacker such a big loan. At the same time, in the current issue, attacker can take and use an additional loan of up to 100M USD in a trustless setup, which will always be possible.
We are talking about an actor that has >200M to use on a governance take over attack, I do not think it is fair to call that actor a noname. Anybody with 200M to spend on a governance take over attack can most likely get 11% more either on or off-chain
Your argument is that you can get 11% leverage which makes the governance take over a little cheaper. How does your calculations change if leverage is not possible? The same attack is still possible but costs ~11% more. So your mitigation does not solve the issue.
Also cfg.maxLine is an admin parameter, doesn't this make this out of scope to begin with?
bb22ff
commented
3 weeks ago @Audittens what is the vulnerability here? It seems everything works as intended:
Is the vulnerability that the current hat can be voted out? Voting for a new hat is not itself an attack. There is nothing special in any presiding hat , they are not a holy monarch. MKR votes is the only source of their legitimacy, so if someone votes with more MKR they bought with their money - they "deserve" the hat, as intended in such a design.
Audittens
commented
3 weeks ago Also cfg.maxLine is an admin parameter, doesn't this make this out of scope to begin with?
No, it doesn't, because as I've already stated in several of my comments, cfg.maxLine is clearly defined to be 100M according to the contest rules.
We are talking about an actor that has >200M to use on a governance take over attack, I do not think it is fair to call that actor a noname. Anybody with 200M to spend on a governance take over attack can most likely get 11% more either on or off-chain.
The majority of the attacks are performed from anonymous actors. That what I meant by "noname". And for trusted setup (which is the only workable way due to necessity of taking undercollateralized debt, no matter where, either on or off chain) it's not likely that it'll be possible to get such a big loan while staying anonymous.
Your argument is that you can get 11% leverage which makes the governance take over a little cheaper. How does your calculations change if leverage is not possible? The same attack is still possible but costs ~11% more. So your mitigation does not solve the issue.
First of all, 11% is the theoretical minimum value. In fact, the exact discount on the attack cost varies from 11% to $(1 - \frac{126}{226}) \sim 44$%. I don't think it's fair to consider only the extreme value that appears when setting Ilk.mat to theoretical maximum of the 1000%. If it was the view of the project to set Ilk.mat to 1000%, I suppose they would extend the range of valid Ilk.mat values by increasing the upperbound to give more flexibility in setting Ilk.mat. But ok, this is more a philosophical discussion, and let's indeed consider the discount of 11% for later arguments:
"So your mitigation does not solve the issue". In the contest rules it's clearly stated several times (1, 2, 3), that issue that exist in the original codebase is considered not an issue and therefore is out of scope. My recommendation successfully mitigates the new issue that arises from introducing LockstakeEngine, and makes the cost of the attack equal to the corresponding cost for the current working system. Since for the current working system, the corresponding attack with larger cost is considered known and the risk is accepted, my recommendation mitigates the issue to the known level acceptable by the project.Audittens what is the vulnerability here? It seems everything works as intended
Please read the description of the issue and elaboration comments. I think the answer to your question is already present in both the original issue and several comments, so I don't think it's reasonable to additionally respond to this question.
WangSecurity
commented
3 weeks ago Actually, in that case, I agree with @0xjuaan and @bb22ff. In this case, indeed the user borrows MKR as they should, but uses this MKR for voting on governance. In other words, they use all the MKR they have to vote on completely viable governance, even though this MKR was received via leverage. This is similar to the user obtaining a large amount of MKR (without leverage) so they can essentially decide on the voting, e.g. if a whale has the majority of MKR.
Moreover, this situation will be noticeable to other MKR holders, so even if the malicious actor momentarily gains access to the hat, any elected action can be dropped by a new hat gathering new MKR holders that want to save their project and their holdings. That new hat could also intend to confiscate the MKR from this malicious entity which won't have other option than deleverage and pay exit fees, making this type of attack nearly impossible to carry without risking huge amounts of money. Also, if at the moment an attack like this happens, MKR will lose a lot of value making the position of the attacker to be liquidated losing exit and penalty fees and making it even easier for the rest to get the hat.
Additionally, to execute such an attack, the attacker has to have 75K MKR (which afaik there's no evidence of any whale that has this) risking it all on an extremely possible liquidation.
Hence, I agree with the Watsons above that this is the design and I believe it's more of a trade-off that users can vote with MKR acquired from leverage. And, in reality, this attack is impossible and in this case, the current holders of the MKR could buy even more MKR to battle the attacker and gather new MKR holders. Moreover, this attack would lead to a drop of MKR value resulting in the attacker's position being liquidated and losing their MKR. I believe this should remain invalid, planning to reject the escalation and leave the issue as it is.
WangSecurity
commented
3 weeks ago Result: Invalid Unique
sherlock-admin4
commented
3 weeks ago
Audittens
High
Acquiring large amounts of MKR via leverage allows to unfairly utilize it
Summary
In the LockstakeEngine it's possible to lock and utilize large amounts of MKR through leverage by looping.
Vulnerability Detail
If a user locks (deposits) MKR into LockstakeEngine, draws (takes a loan of) DAI, converts it back into MKR (e.g. using a Uniswap pair), and locks that MKR back, they achieve leverage by looping these operations. Considering
matin Ilk of LockstakeEngine to be set to a reasonable value of ~150%, max leverage can be estimated as $LEV = \sum\limits_{i = 0}^{\infty} \frac{1}{1.5^i} = \frac{1}{1 - \frac{1}{1.5}} = 3$. So for each real MKR a user initially has, they can get about 3 MKR locked in LockstakeEngine, and use that locked MKR for VoteDelegate and Farm. In common cases of leverage, the user only gains these 2 extra MKR "on paper" and therefore is unable to use them in any way, except for being more exposed to the price movements. However, in our case the user will be able to loop and gain 3x leverage on locked MKR, which they can use in both VoteDelegate and Farm.Impact
Users who utilize leverage: 1) will have three times more voting power than intended, which allows them to gain a majority of the voting power while initially holding just a portion of the MKR. Let’s say that some user has 17% of total amount of MKR participating in voting. Through leverage, they will gain 51% locked in LockstakeEngine, while other users combined only have 49%, making that user the majority holder of voting power. This leads to our user being able to execute any spell they would like to. For example, they could transfer all tokens that the PauseProxy holds to their own address, which totals to about 170 millions of USD, as of August 5th, 2024, while 17% of MKR participating in voting totals to about 43 millions of USD, as of August 5th, 2024. 2) will receive farming rewards, that are three times larger, at the expense of other users, leading to a loss of funds for those users.
Code Snippet
https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeEngine.sol#L260-L272 https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeEngine.sol#L287-L295 https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeEngine.sol#L309-L313 https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/lockstake/src/LockstakeEngine.sol#L382-L389
Tool Used
Manual Review
Recommendation
Don't allow user to draw (take a loan), while he has active VoteDelegate or Farm. This change ensures the user is unable to use locked MKR gained by leverage for unfair voting or Farm.