Submitted by pontifex.
Since the MErc20DelegateFixer.fixUser function bypasses the MToken.transferTokens function the liquidator's rewardSupplierIndex is not updated. So the rewards calculation will count the new mToken balance of the liquidator for the whole period since the previous reward distribution. Anyone can call The Comptroller.claimReward function for any user. This way the wrong amount of rewards will be distributed to the liquidator balance (multisig) besides of the wish of DAO.
MErc20DelegateFixer.fixUser function transfers mTokens from liquidated account to the liquidator's address:
/// @dev current amount for a user that we'll transfer to the liquidator
uint256 liquidated = accountTokens[user];
/// can only seize collateral assets if they exist
if (liquidated != 0) {
/// if assets were liquidated, give them to the liquidator
accountTokens[liquidator] = SafeMath.add(
accountTokens[liquidator],
liquidated
);
/// zero out the user's tokens
delete accountTokens[user];
}
The common flow for all transfers includes call to the Comptroller contract to check transfer allowance and to distribute rewards for previous period:
function transferAllowed(address mToken, address src, address dst, uint transferTokens) external returns (uint) {
// Pausing is a very serious situation - we revert to sound the alarms
require(!transferGuardianPaused, "transfer is paused");
// Currently the only consideration is whether or not
// the src is allowed to redeem this many tokens
uint allowed = redeemAllowedInternal(mToken, src, transferTokens);
if (allowed != uint(Error.NO_ERROR)) {
return allowed;
}
// Keep the flywheel moving
updateAndDistributeSupplierRewardsForToken(mToken, src);
updateAndDistributeSupplierRewardsForToken(mToken, dst);
return uint(Error.NO_ERROR);
}
There are no liquidator's rewards claim in the MErc20DelegateFixer.fixUser and in the mipm17 proposal. So after the proposal execution the liquidator can receive additional reward tokens because of the incorrect period. This can cause different issues such as an unexpected voting power of the liquidator account (multisig).
Incorrect rewards distribution, unexpected behavior.
Manual Review
Consider calling the Comptroller.claimReward function for the liquidator account before mToken transfer.
getCashPrior() function returns incorrect amount of funds owned by the protocol.Submitted by ljj.
Updated getCashPrior() function returns incorrect amount of funds owned by the protocol.
getCashPrior() function is changed in order to not have an impact on the exchange rate with the update.
Current version:
function getCashPrior() internal view returns (uint256) {
return EIP20Interface(underlying).balanceOf(address(this));
}
Updated version
function getCashPrior() internal view returns (uint256) {
return EIP20Interface(underlying).balanceOf(address(this)) + badDebt;
}
This causes functions relying solely on getCashPrior() without using totalBorrows will receive incorrect data, leading to those functions not reverting when they should. Namely these 3 functions are: borrowFresh(), _reduceReservesFresh() and redeemFresh()
These functions have the following if check to see if the protocol has enough funds to support the transaction.
if (getCashPrior() < vars.redeemAmount) {
return fail(Error.TOKEN_INSUFFICIENT_CASH, FailureInfo.REDEEM_TRANSFER_OUT_NOT_POSSIBLE);
}
However after fixUser()calls, in an example scenario where a user calls redeemFresh() with amount of tokens that exceeds the tokens protocol has, updated getCashPrior() will return a uint bigger than the amount of tokens the protocol has. This means that the if check here will be bypassed when it should be returning the error message provided in this function. In this scenario the function would revert at the following external call and receive the wrong error message.
doTransferOut(redeemer, vars.redeemAmount);
This vulnerability causes user’s to get the wrong error message on why their transaction has failed, shows the wrong amount of tokens owned by the protocol until the badDebt is paid. This can cause bigger issues on the front-end systems. Which is why this is a low impact vulnerability.
Manual review.
borrowFresh(), _reduceReservesFresh() and redeemFresh() should be updated to read
EIP20Interface(underlying).balanceOf(address(this))
instead of getCashPrior() function.
A better solution would be implementing a new function to be used in exchange rate maths that includes badDebt and leaving getCashPrior() as it is.