CarefulMath / safe math not allways used

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

CarefulMath is used in most calculations, however it isn't always used. Although I didn't spot any real issues with this it isn't consistent.

Proof of Concept

.\Governance\Blo.sol: return nCheckpoints > 0 ? checkpoints[account][nCheckpoints - 1].votes : 0; .\Governance\Blo.sol: if (checkpoints[account][nCheckpoints - 1].fromBlock <= blockNumber) { .\Governance\Blo.sol: return checkpoints[account][nCheckpoints - 1].votes; .\Governance\Blo.sol: uint32 upper = nCheckpoints - 1; .\Governance\Blo.sol: upper = center - 1; .\Governance\Blo.sol: uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0; .\Governance\Blo.sol: uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0; .\Governance\Blo.sol: if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) { .\Governance\Blo.sol: checkpoints[delegatee][nCheckpoints - 1].votes = newVotes; .\CToken.sol: totalReservesNew = totalReserves - reduceAmount; .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - newObservation.timestamp; .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - oldTimestamp; .\UniswapOracle\UniswapAnchoredView.sol: FixedPoint.uq112x112 memory priceAverage = FixedPoint.uq112x112(uint224((nowCumulativePrice - oldCumulativePrice) / timeElapsed)); .\UniswapOracle\UniswapAnchoredView.sol: uint timeElapsed = block.timestamp - newObservation.timestamp; .\CEther.sol: fullMessage[i+2] = byte(uint8(48 + ( errCode / 10 ))); .\CEther.sol: fullMessage[i+3] = byte(uint8(48 + ( errCode % 10 ))); .\DAIInterestRateModelV3.sol: gapPerBlock = 4e16 / blocksPerYear; .\DAIInterestRateModelV3.sol: gapPerBlock = gapPerYear / blocksPerYear; .\UniswapOracle\UniswapAnchoredView.sol: return mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: price = mul(usdPerEth, config.fixedPrice) / ethBaseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(1e30, price) / config.baseUnit; .\UniswapOracle\UniswapAnchoredView.sol: return mul(1e30, priceInternal(config)) / config.baseUnit; .\UniswapOracle\UniswapAnchoredView.sol: FixedPoint.uq112x112 memory priceAverage = FixedPoint.uq112x112(uint224((nowCumulativePrice - oldCumulativePrice) / timeElapsed)); .\UniswapOracle\UniswapAnchoredView.sol: anchorPrice = mul(unscaledPriceMantissa, config.baseUnit) / ethBaseUnit / expScale; .\UniswapOracle\UniswapLib.sol: return uq112x112((uint224(numerator) << 112) / denominator); .\UniswapOracle\UniswapLib.sol: return uint(self._x) / 5192296858534827; .\UniswapOracle\UniswapLib.sol: return uint32(block.timestamp % 2 * 32); .\UniswapOracle\UniswapLib.sol: uint32 timeElapsed = blockTimestamp - blockTimestampLast; .\UniswapOracle\UniswapLib.sol: price0Cumulative += uint(FixedPoint.fraction(reserve1, reserve0)._x) timeElapsed; .\UniswapOracle\UniswapLib.sol: price1Cumulative += uint(FixedPoint.fraction(reserve0, reserve1)._x) * timeElapsed;

Tools Used

grep

Recommended Mitigation Steps

Double check to see if safe math functions really are not necessary and otherwise add a comment

[ghoul-sol]

Agreed, however, in my opinion it's a non-critical issue.

[cemozerr]

I will rate this as low risk instead of non-critical because although the warden here might not have spotted any real issues, unless CarefulMath / SafeMath is not always used, there might be hidden underflow/overflow bugs.