search

code-423n4 / 2021-06-gro-findings

0 stars 1 forks source link

distributePriceChange might revert #21

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The function distributePriceChange includes the following statement: lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(totalAssets));

If you look at this: lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(gvtAssets.add(pwrdAssets))); lastGvtAssets = gvtAssets + currentTotalAssets- gvtAssets -pwrdAssets lastGvtAssets = currentTotalAssets - pwrdAssets (with checking of underflows via safemath) Then you can see it will revert if currentTotalAssets < pwrdAssets In that situation the administration will not be updated appropriately

Proof of Concept

//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L282 function distributePriceChange(uint256 currentTotalAssets) external override { require(msg.sender == controller, "!Controller"); uint256 gvtAssets = lastGvtAssets; uint256 pwrdAssets = lastPwrdAssets; uint256 totalAssets = gvtAssets.add(pwrdAssets); if (currentTotalAssets > totalAssets) { lastGvtAssets = gvtAssets.add(currentTotalAssets.sub(totalAssets)); } else if (currentTotalAssets < totalAssets) { (lastGvtAssets, lastPwrdAssets) = handleLoss(gvtAssets, pwrdAssets, totalAssets.sub(currentTotalAssets)); } int256 priceChange = int256(currentTotalAssets) - int256(totalAssets); ... }

Tools Used

Recommended Mitigation Steps

Detect the situation where currentTotalAssets < pwrdAssets and take appropriate recovery actions.

kitty-the-kat commented 3 years ago

currentTotalAssets = pwrdAssets + gvtAssets, so currentTotalAssets will always be >= pwrdAssets + 1

ghoul-sol commented 3 years ago

As explained by sponsor, issue is invalid.