0x73696d616f - `LenderCommitmentGroup_Smart` does not use `mulDiv` when converting between token and share amounts, possibly leading to DoS or loss of funds
Opened this issue · 14 comments
0x73696d616f
medium
LenderCommitmentGroup_Smart does not use mulDiv when converting between token and share amounts, possibly leading to DoS or loss of funds
Summary
LenderCommitmentGroup_Smart calculates the exchange rate and _valueOfUnderlying() without using mulDiv from OpenZeppelin, which might make it overflow, leading to DoS and possible loss of funds.
Vulnerability Detail
sharesExchangeRate() is calculated as
rate_ =
(poolTotalEstimatedValue *
EXCHANGE_RATE_EXPANSION_FACTOR) /
poolSharesToken.totalSupply();`which overflows if poolTotalEstimatedValue > (2^256 - 1) / 1e36. The readme mentions that any ERC20 token that is supported by a Uniswap pool is supported, which means that if a token has decimals of, for example, 1e36, the calculation above may easily overflow. In this case, rate_ = realPoolTotalEstimatedValue * 1e36 * 1e36 = realPoolTotalEstimatedValue * 1e72, where realPoolTotalEstimatedValue is the number of tokens without the decimals part. Thus, the number of tokens needed to overflow would be (2^256 - 1) / 1e72 = 115792, which at a price of $1 is just 115792 USD.
This bug is also present in _valueOfUnderlying(), which returns value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate;.
Impact
DoS of LenderCommitmentGroup_Smart, possibly with stuck tokens if users call addPrincipalToCommitmentGroup() with smaller amount at a time that do not cause an overflow when calculating sharesExchangeRate() and _valueOfUnderlying(), but would overflow when withdrawing in the calculation of sharesExchangeRate(), as poolTotalEstimatedValue could have increased to the value that overflows.
Code Snippet
function sharesExchangeRate() public view virtual returns (uint256 rate_) {
...
rate_ =
(poolTotalEstimatedValue *
EXCHANGE_RATE_EXPANSION_FACTOR) /
poolSharesToken.totalSupply();
}function _valueOfUnderlying(uint256 amount, uint256 rate)
internal
pure
returns (uint256 value_)
{
...
value_ = (amount * EXCHANGE_RATE_EXPANSION_FACTOR) / rate;
}Tool used
Manual Review
Vscode
Recommendation
Use Math::mulDiv from OpenZeppelin.
The protocol team fixed this issue in the following PRs/commits:
teller-protocol/teller-protocol-v2-audit-2024#14
request poc
Hi watson could you provide me a realistic current token that is supported on uniswapV3 that can cause this type of overflow?
PoC requested from @0x73696d616f
Requests remaining: 7
Hi @nevillehuang, found the following token that has 32 decimals and is on a Uniswap pool, BBC. (2^256-1) / 1e36 / 1e32 is approx 1.15e9, which means that if the token is worth $0.01, it would require 11.5 million USD in TVL to lock the contract due to overflow.
Escalate
Should be invalid. Such high token decimals are unrealistic. Even if the recommendation is followed, it can always be said that protocol can't operate with 60 decimals tokens. Also, the given example is a random inactive token with a total of 32 transactions.
Escalate
Should be invalid. Such high token decimals are unrealistic. Even if the recommendation is followed, it can always be said that protocol can't operate with 60 decimals tokens. Also, the given example is a random inactive token with a total of 32 transactions.
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.
60 decimals is not realistic, but 30ish is. We do not know the reason for the 32 transactions but the token exists and proves my point.
The way to determine what is realistic is by observing what is used in practice. There is no token with remarkable activity with 30-ish decimals. You're purely speculating.
There is no token with remarkable activity with 30-ish decimals.
You do not have the data to back this up, and I have found one without recent activity that proves this.
You're purely speculating.
Again, we do not know why the token died, but it still proves my point. Anyway here is another token with 27 decimals. (2^256-1) / 1e36 / 1e27 = 1e14. 3/30 = 10 %, which means the token is 10% close to the 30 decimals mark. If the token price crashes, given some TVL, it will overflow. Also, with this token, someone may create an ERC4626 token with _decimalsOffset() of at least 3 and it would reach the 30 decimals mark, again, proving my point. (_decimalsOffset() in ERC4626 tokens is a common way to mitigate inflation attacks and such by increasing the precision of the shares in relation to the assets). OpenZeppelin uses a _decimalsOffset() of 3 in their example, so this is a realistic value (the delta in the images).
This issue is valid as it was proved that it overflows for some tokens.
Since an appropriate example is provided that corresponds to the contest details, I believe this issue should remain valid.
The protocol has written in the Readme that they will use any token compatible with Uniswap V3.
@0x73696d616f has given a valid example of how using a token with more decimals(27+) can lead to DoS.
Planning to reject the escalation and leave the issue as is.
Result:
Medium
Unique
The Lead Senior Watson signed off on the fix.