Wrong calculation of `FrozenCoinAge`

[code423n4]

Handle

WatchPug

Vulnerability details

The current implementation of Comptroller.sol#addFrozenCoinAge() and UserManager.sol#getFrozenCoinAge(), the duration of overdue time is calculated based on the block number of lastRepay and the current block number.

However, since the borrower may have never repaid before the loan went overdue. block.number - lastRepay may contain the overdueBlocks which should not be considered as part of the overdue duration.

This makes the stakers lose rewards.

https://github.com/code-423n4/2021-10-union/blob/4176c366986e6d1a6b3f6ec0079ba547b040ac0f/contracts/token/Comptroller.sol#L205-L221

function addFrozenCoinAge(
    address staker,
    address token,
    uint256 lockedStake,
    uint256 lastRepay
) external override onlyUserManager(token) {
    uint256 lastBlock = users[staker][token].updatedBlock;
    uint256 blocks;
    if (lastBlock > lastRepay) {
        // Frozen CoinAge here has been accounted for when the user withdraws the rewards, so here just need to calculate the delta between block.number and lastBlock
        blocks = block.number - lastBlock;
    } else {
        blocks = block.number - lastRepay;
    }

    users[staker][token].frozenCoinAge += lockedStake * blocks;
}

https://github.com/code-423n4/2021-10-union/blob/4176c366986e6d1a6b3f6ec0079ba547b040ac0f/contracts/user/UserManager.sol#L794-L814

function getFrozenCoinAge(address staker, uint256 pastBlocks) public view override returns (uint256) {
    uint256 totalFrozenCoinAge = 0;

    address[] memory borrowerAddresses = getBorrowerAddresses(staker);

    for (uint256 i = 0; i < borrowerAddresses.length; i++) {
        address borrower = borrowerAddresses[i];
        uint256 blocks = block.number - uToken.getLastRepay(borrower);
        if (uToken.checkIsOverdue(borrower)) {
            (, , uint256 lockedStake) = getStakerAsset(borrower, staker);

            if (pastBlocks >= blocks) {
                totalFrozenCoinAge = totalFrozenCoinAge + (lockedStake * blocks);
            } else {
                totalFrozenCoinAge = totalFrozenCoinAge + (lockedStake * pastBlocks);
            }
        }
    }

    return totalFrozenCoinAge;
}

Recommendation

Change to:

function addFrozenCoinAge(
    address staker,
    address token,
    uint256 lockedStake,
    uint256 lastRepay
) external override onlyUserManager(token) {
    uint256 lastBlock = users[staker][token].updatedBlock;
    uint256 blocks;
    if (lastBlock > lastRepay) {
        // Frozen CoinAge here has been accounted for when the user withdraws the rewards, so here just need to calculate the delta between block.number and lastBlock
        blocks = block.number - lastBlock;
    } else {
        blocks = block.number - (lastRepay + overdueBlocks);
    }

    users[staker][token].frozenCoinAge += lockedStake * blocks;
}

function getFrozenCoinAge(address staker, uint256 pastBlocks) public view override returns (uint256) {
    uint256 totalFrozenCoinAge = 0;

    address[] memory borrowerAddresses = getBorrowerAddresses(staker);

    for (uint256 i = 0; i < borrowerAddresses.length; i++) {
        address borrower = borrowerAddresses[i];
        uint256 blocks = block.number - (uToken.getLastRepay(borrower) + overdueBlocks);
        if (uToken.checkIsOverdue(borrower)) {
            (, , uint256 lockedStake) = getStakerAsset(borrower, staker);

            if (pastBlocks >= blocks) {
                totalFrozenCoinAge = totalFrozenCoinAge + (lockedStake * blocks);
            } else {
                totalFrozenCoinAge = totalFrozenCoinAge + (lockedStake * pastBlocks);
            }
        }
    }

    return totalFrozenCoinAge;
}

[kingjacob]

If a borrower is a "first payment defaulter" they technically defaulted when they borrowed.

[GalloDaSballo]

In the specific case mentioned by the warden, the code would run:

function addFrozenCoinAge(
    address staker,
    address token,
    uint256 lockedStake,
    uint256 lastRepay
) external override onlyUserManager(token) {
    uint256 lastBlock = users[staker][token].updatedBlock;
    uint256 blocks;
    if (lastBlock > lastRepay) {
        // Frozen CoinAge here has been accounted for when the user withdraws the rewards, so here just need to calculate the delta between block.number and lastBlock
        blocks = block.number - lastBlock;
    } else {

And it would always be the case that lastBlock (last accrue) is > lastRepay (block0, never repaid)

I don't fully understand the message from the sponsor, however I agree in saying that for this to happen, the account would have to self-liquidate or run in an incorrect set of steps, which at this time I don't believe possible