code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1084-L1112

Vulnerability details

Impact

The total number of tokens to be seized could be wrongly calculated if the underlying assets of vTokenBorrowed & vTokenCollateral have a different decimals.

Proof of Concept

The price returned by the ChainlinkOracle contract of the Venus Protocol, the price is returned in the scale of 10 ^ (36 - underlying asset decimals))

Example

    WBNB (18 decimals in BSC).  USD price: $300.    Value returned by the Oracles:  300^1e18
    TRX (6 decimals in BSC).    USD price: $0.068.  Value returned by the Oracles:  0.068^1e30

That means, that the priceBorrowedMantissa will be scaled by a magnitude depending on the decimals of the underlying asset.
Same for the priceCollateralMantissa, it will be scaled by a magnitude depending on the decimals of the underlying asset.
The problem is when the two underlying assets uses different decimals, the two prices will be scaled up by a total different magnitude.
Then, in the rest of the math operations to estimate the seize tokens, any of the two prices are normalized in order to make the two magnitudes of the prices equals, so the tokens to be seized are estimated correctly.
Depending on the number of decimals of the two underlying assets, the calculated amount of tokens to be seized it might be either greater or lower than it really should be.

Tools Used

Recommended Mitigation Steps

The recommendation to prevent this issue is to normalize the amount of the two underlying assets before operating between the two, that means, when calculating the numerator and denominator, make sure to divide by 1e18 so the two values are now scaled by the same magnitude.

Assessed type

Math

0xean commented 1 year ago

similar to #468 but in a different part of the codebase.

c4-sponsor commented 1 year ago

chechu marked the issue as sponsor disputed

chechu commented 1 year ago

The maths seems correct. It should be taken into account that exchangeRate has a different number of decimals depending on the underlying decimals. Example:

borrow WBNB (18 decimals). priceBorrowMantissa: 300 * 1e18
collateral TRX (6 decimals). priceCollateralMantissa: 0.068 * 1e30
user position: 100M TRX supplied (unique supplier) & 210 BNB borrowed
user position in $: $68K TRX supplied & $63K BNB borrowed
liquidationThreshold: 0.9. So, position underwater, eligible to be liquidated
initial exchangeRate(vTRX): 1e16 (set during initialization)
exchangeRate(vTRX) after suppling 100M TRX: 1e16
liquidationIncentive: 1.1 * 1e18 (set during initialization)
repayAmount: 105 BNB (50% of the debt)

—- the pen & paper numbers:

liquidator repay 105 BNB and they receive TRX equivalent to 115.5 BNB (105 * 1.1)
seizedAmount: 115.5 * (300 / 0.068) = 509,558.82 full TRX

—- now applying the formulas coded in the contract, numerator: 300 1.1 1e18 denominator: 0.068 1e28 ratio: _div(300 1.1 1e18 , 0.068 1e28) = (300 1.1 1e8) / 0.068 seizedTokens: mul_ScalarTruncate((300 1.1 1e8) / 0.068 , 105 1e18) = 509,558 1e8 vTRX

given the exchange rate (1e16), and the formula underlying = mul_ScalarTruncate(exchangeRate, vTokens), 509,558 1e8 vTRX are 509,558 1e6 TRX (atoms). The same amount calculated with pen & paper

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-05-venus-findings

Incorrectly calculation of the total tokens to be seized because of the difference on the scale of magnitude for the prices of the underlying assets #551