Venus Protocol Isolated Pools - LokiThe5th's results

0xean marked the issue as duplicate of #314

0xean marked the issue as satisfactory

0xean changed the severity to 3 (High Risk)

0xean changed the severity to 2 (Med Risk)

0xean marked the issue as selected for report

1. Allows the attacker to push their leverage past the market collateralFactor

This is wrong. If you mint (receiving X vTokens) and then you transfer underlying tokens to the market, your X vTokens will have a greater value because the exchange rate is greater after the donation. In the PoC:

1. The attacker mints 10 WBTC -> vTokens received: 10
2. The attacker donates 20 WBTC -> this change the exchange rate from 1000000000000000000 to 2000000000000000000, so, basically, the vTokens previously minted now can be redeemed receiving the double amount of WBTC (that is the expected effect of this donation)

So, the value of the 10 vTokens of the attacker after the donation is 20 WBTC, not 10 WBTC. For that reason the user can borrow 13 WBTC (13 < 20 * 0.7, where 0.7 is the collateral factor).

The attacker could get a similar (actually better) effect minting more vTokens supplying the 20 WBTC tokens, instead of donating them. Moreover, donating is a benefit for every user with vTokens before the donation, while minting only benefits the minter.

To demonstrate it, you can replace the following statement in the PoC:

await mockWBTC.connect(attacker).transfer(vWBTC.address, convertToUnit(20, 8))

with this one:

await vWBTC.connect(attacker).mint(convertToUnit(20, 8));

And the output of the PoC will be the same.

2. Violates the internal accounting when borrowing and repaying causing the totalBorrows and the sum of the individual account borrows to become out of sync, i.e. the totalBorrows can become 0 while their are still some loans outstanding, leading to loss of earned interest.

This is not because of the transfer (donation), but because of the known rounding issues associated with the used maths, that can generate small differences among the totalBorrow variable and the sum of the individual borrowed amounts.

3. The attacker can use the leverage to repeatedly borrow + mint into the VToken in order to inflate their share of the token rewards issued by the rewards distributor.

That is true, but, again, it's independent of the donations. Users can use the leverage to increase their positions and therefore get more rewards. The main downside of leveraging is the cost (every X WBTC borrowed that are then supplied implies a cost for the user proportional to the reserve factor of the market). So, taking into account that the total rewards to distribute are fixed, the leverage can make sense depending on the total suppliers and borrowers.

The permanent side-effect of this exploit is that the minting of VTokens to any subsequent users remains stunted as there is no direct mechanism to clear the excess underlying tokens from the contract. This can taint this Pool permanently.

Donations to markets are supported, and there aren't known negative side effects on regular scenarios. If the liquidity of the market is very low, donations can facilitate issues related to rounding (like in the Hundred Finance attack, https://twitter.com/danielvf/status/1647329491788677121), but every market in Venus starts with a minimum liquidity that should reduce these risks.

Finally, in the PoC, some redeems operations fail because those users still have some borrowed amount. Printing the error thrown you can see how the error is InsufficientLiquidity, thrown in the Comptroller._checkRedeemAllowed function. To repay 100% of the debt, the best option is to invoke the repayBorrow providing an big amount as parameter. The function will get only the borrowed amount (considering interest accrued until that block).

Sponsor requested additional feedback from the warden in regards to this submission after the Post-Judging QA period. C4 staff reached out to the warden directly with that request.

LokiThe5th (warden) response:

Thank you for the feedback. I don't have access to my original notes anymore, but will try to provide clarity where I can. To be clear, in retrospect, this submission seems to have conflated a few issues while trying to demonstrate the exchange rate issue.

This is wrong. If you mint (receiving X vTokens) and then you transfer underlying tokens to the market, your X vTokens will have a greater value because the exchange rate is greater after the donation.

Yes, you are correct. The exchange rate is manipulated (which is the issue). To be more specific, the attacker appears to be able to push past the collateral factor when considering the amount of vToken held by the attacker. The intention here is to demonstrate this exchange rate manipulation through borrowing past what the internal accounting would hold the attacker's safe collateral factor would be.

This is not because of the transfer (donation), but because of the known rounding issues associated with the used maths, that can generate small differences among the totalBorrow variable and the sum of the individual borrowed amounts.

Indeed, rounding in Solidity is a known issue. It may well be that the direct transfer only served to exacerbate this issue when compared with the control scenario.

That is true, but, again, it's independent of the donations. Users can use the leverage to increase their positions and therefore get more rewards. The main downside of leveraging is the cost (every X WBTC borrowed that are then supplied implies a cost for the user proportional to the reserve factor of the market). So, taking into account that the total rewards to distribute are fixed, the leverage can make sense depending on the total suppliers and borrowers.

You are correct that this is independent of donations. Users using leverage in this way to increase their rewards is likely a separate issue.

Donations to markets are supported, and there aren't known negative side effects on regular scenarios. If the liquidity of the market is very low, donations can facilitate issues related to rounding (like in the Hundred Finance attack, https://twitter.com/danielvf/status/1647329491788677121), but every market in Venus starts with a minimum liquidity that should reduce these risks.

In the context of modular markets exchange rate manipulation can be damaging. It is good practice to explicitly handle (or not handle) donations in the accounting logic for the contract. For example in the standard UniswapV2Pair contracts calculations are made using an internal tracking of reserves to avoid this issue.

Finally, in the PoC, some redeems operations fail because those users still have some borrowed amount. Printing the error thrown you can see how the error is InsufficientLiquidity, thrown in the Comptroller._checkRedeemAllowed function. To repay 100% of the debt, the best option is to invoke the repayBorrow providing an big amount as parameter. The function will get only the borrowed amount (considering interest accrued until that block).

You are correct. Some redeems fail because some users still have borrowed amounts. In the preceeding code these users tried repay their borrows using their exact borrowBalance from the call to VToken.getAccountSnapshot(user). It would be acceptable for a user to assume that should they try to repayBorrow with this outstanding amount. If memory serves this failure of repayment using the returned borrowBalance happened in exchange manipulation scenarios, but not others, but this may have been a mistaken assumption if that is not the case. If so, it would also be a separate issue.

chechu (Venus) response:

Hey! Thanks for your message.

To be more specific, the attacker appears to be able to push past the collateral factor when considering the amount of vToken held by the attacker

I think that is not precise. The donation doesn't allow users to break the rule of the collateral factor. The donation is increasing the value of the vTokens, so any user with vTokens before the donation will be able to borrow more tokens. That is correct, expected, and don't generate any issue.

You call it manipulation, and I can see your point because with a donation the user is able to change the value of the exchange rate. Personally, I don't think this is a manipulation, because the user doesn't get any benefit by doing it. As I said, if the attacker mints instead of donating the same amount, he would be able to borrow more tokens (in a regular scenario, not being the first and only supplier).

Example:

• Initial exchange rate: 1 (1 underlying token == 1 vToken)
• User 1 mints 1,000 tokens, receiving 1,000 vTokens. Exchange rate is not affected, so, it's 1
• Attacker 1 mints 1,000 tokens, receiving 1,000 vTokens. Exchange rate is not affected, so, it's 1
• Attacker 1 donates 2,000 tokens, not receiving anything, but changing the exchange rate, that now will be 2 (total cash / total vTokens minted)

So, now the 1,000 vTokens have more value (the attacker would be able to redeem 1,000 vTokens and receive 2,000 tokens, instead of the original 1,000 tokens he minted). And therefore, the "borrowing power" of the attacker is greater. The attacker can borrow more assets from another market, because now his 1,000 vTokens has more value.

But, that is a bad strategy by the attacker, because by doing the donation User 1 also received a benefit. Now, User 1 can redeem his 1,000 vTokens, receiving 2,000 tokens. Not only his original 1,000 tokens.

A better strategy by the attacker would be to mint 2,000 tokens, instead of donating them. This way, the exchange rate doesn't change (so User 1 doesn't receive any benefit) and the "borrowing power" of the attacker is even higher (3,000 tokens, instead of 2,000 tokens achieved via the donation).

So, yes, with a donation you are able to update the exchange rate, but you won't get any benefit, and you will lose resources.

Indeed, rounding in Solidity is a known issue. It may well be that the direct transfer only served to exacerbate this issue when compared with the control scenario.

If you mint instead of donating, the rounding issue appears too. So, I don't think the donation exacerbates the rounding issue.

In the context of modular markets exchange rate manipulation can be damaging. It is good practice to explicitly handle (or not handle) donations in the accounting logic for the contract. For example in the standard UniswapV2Pair contracts calculations are made using an internal tracking of reserves to avoid this issue.

In the Venus protocol, I think donations benefit every vToken holder and don't affect future holders, because for a user getting vTokens, the relevant events happen from the vTokens are minted until they are redeemed. It doesn't matter what happened before. Moreover, the exchange rate is never decreasing. So, IMO, we can avoid the internal tracking of cash in the markets.

this failure of repayment using the returned borrowBalance happened in exchange manipulation scenarios, but not others

I think the failures of repayments are associated with the rounding issues, not with donations. I modified the provided PoC, transforming the donation into a mint, and this issue is still there. I think the impact is low because users are not able to repay 100% of their debt only in edge cases, with 1 or 2 borrowers in the market and after several blocks. With a regular number of borrowers, users shouldn't have any problem repaying their debt, and therefore redeeming their vTokens

Thank you again for your time reviewing the code. We really appreciate it. Your comments push us to improve the code (and to understand it better, tbh). We are totally open to trying to clarify any doubts :)