Handle

hyh

Vulnerability details

Impact

Impact depends on subtraction overflow handling and this way on the compiler version used for production deployment.

If compiler version above 0.8:

The compiler will check subtraction and fail, so:
a user will have all withdrawals failed most of the times, the funds will be frozen within the contract as both withdraw functions, withdrawAccountRewards and withdrawRewardsForAccount, rely on _withdrawMultiple.

If compiler version less than 0.8, but above 0.7:

MiningService will not inherit Permissions contract usage of SafeMath for uint256, so:
a user will end up withdrawing all the funds held with mines no matter what amount was requested.

<https://docs.soliditylang.org/en/latest/070-breaking-changes.html#functions-and-events&gt;

using A for B only affects the contract it is mentioned in. Previously, the effect was inherited. Now, you have to repeat the using statement in all derived contracts that make use of the feature.

If compiler version less than 0.7:

MiningService will inherit Permissions contract usage of SafeMath for uint256, so:
a user will have all withdrawals failed most of the times, the funds will be frozen within the contract.

Proof of Concept

Most of the contracts use pragma solidity >=0.6.6, so does MiningService.

MiningService._withdrawMultiple performs withdrawals in the cycle, withdrawing from all available mines, decreasing amount each time with reward amount received.

The condition to stop withdrawals is

if (amount == 0) {
	break;
}

<https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MiningService.sol#L165&gt;

When sub operation is not checked, and as stop condition is exact equality, the following will happen, depending on complier version:

If withdrawnAmount from a mine will be such that withdrawnAmount > amount, the amount = amount.sub(withdrawnAmount) will

If compiler version is above 0.8 or below 0.7:
throw and the withdrawal be stopped.

If compiler version is between 0.7 and 0.8:
cause overflow and the amount == 0 condition will be never met, so a user will end up withdrawing all the funds held with mines no matter what amount was requested.

As the sum of withdrawnAmounts will not exactly match amount requested most of the times, this will be the typical behavior of the function.

Recommended Mitigation Steps

Change stop condition to inequality:

Now:

amount = amount.sub(withdrawnAmount);

if (amount == 0) {
	break;
}

To be:

if (amount > withdrawnAmount + dust) {  // dust is some precision threshold to avoid another withdrawal when the diff is negligible
	amount = amount.sub(withdrawnAmount);
} else {
	break;
}

---

The text was updated successfully, but these errors were encountered:

All reactions