DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

BridgeRouterFacet::withdraw() and unstake() can revert when amount * TVL > uint88 because of PRBMathHelper::mulU88

infect3d

Summary

When the protocol will be well established, there might be user wanting to withdraw substantial amount of ETH, but the call will revert because of an overflow that shouldn't happen.

Vulnerability Details

In withdraw() or unstakeEth(), the amount that the user want to get is converted from the LSD to ETH by calling the function _ethConversion(...) :

function _ethConversion(uint256 vault, uint88 amount) private view returns (uint88) {
uint256 zethTotalNew = vault.getZethTotal();
uint88 zethTotal = s.vault[vault].zethTotal;
​
​
if (zethTotalNew >= zethTotal) {
// when yield is positive 1 zeth = 1 eth
return amount;
} else {
// negative yield means 1 zeth < 1 eth
return amount.mulU88(zethTotalNew).divU88(zethTotal); //@audit revert if amount*zethTotalNew > u88, replace mulU88 by mul
}
}

But as we can see in the else case, amount is multiplied by zethTotalNew using the PRBMathHelper::mulU88 function.
This function reverts when the result of the calculation is greater than U88.
But there is realistic situations (amount: 333 ETH, zethTotalNew: 1M ETH, see total LSD staked here: 11.3M ETH staked) where this can happen.
The thing is that what matters in this calculation, is that the final result fits in a U88.
That means, only the division should check the overflow, but its not the case.

Impact

User request to withdraw or unstake will revert.

Proof of Concept

Add this test to Yield.t.sol:

function test_audit_withdrawInvalidAmount() public {
uint88 revertWithdrawal = 333 ether;
uint88 zethTotal = 1_000_000 ether;
uint88 zethTotalNew = 1_000_001 ether;
deal(_reth, sender, revertWithdrawal+100);
deal(_reth, extra, zethTotal);
​
vm.prank(extra);
diamond.deposit(_bridgeReth, zethTotal);
​
vm.prank(sender);
diamond.deposit(_bridgeReth, revertWithdrawal+100);
​
deal(_reth, _bridgeReth, zethTotalNew); // Mimics loss of value of 0.1 ETH
​
vm.prank(sender);
diamond.withdraw(_bridgeReth, revertWithdrawal);
}

Tools Used

Manual review

Recommended Mitigation Steps

replace the first mulU88 by a mul:

diff --git a/contracts/facets/BridgeRouterFacet.sol b/contracts/facets/BridgeRouterFacet.sol
index c9ff4e5..3680ae6 100644
--- a/contracts/facets/BridgeRouterFacet.sol
+++ b/contracts/facets/BridgeRouterFacet.sol
@@ -176,12 +176,12 @@ contract BridgeRouterFacet is Modifiers {
function _ethConversion(uint256 vault, uint88 amount) private view returns (uint88) {
uint256 zethTotalNew = vault.getZethTotal();
uint88 zethTotal = s.vault[vault].zethTotal;
if (zethTotalNew >= zethTotal) {
// when yield is positive 1 zeth = 1 eth
return amount;
} else {
// negative yield means 1 zeth < 1 eth
- return amount.mulU88(zethTotalNew).divU88(zethTotal);
+ return amount.mul(zethTotalNew).divU88(zethTotal); //@audit-issue revert if amount*zethTotalNew > u88, replace mulU88 by mul
}
}
Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other
infect3d Submitter
almost 2 years ago
infect3d Submitter
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
infect3d Submitter
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-633

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.