Users can lose their staking rewards.
By following the steps described in `Vulnerability Detail`, user is able to lose all of his staking rewards.\\nThe issue occurs in the following steps described below:\\nKiki calls the function `unstake` and unstakes all of his funds, as a result the internal function `_updateStakedCoinAge` is called to update his staked coin age till the current block.\\n```\\ncontracts/user/UserManager.sol\\n\\n function unstake(uint96 amount) external whenNotPaused nonReentrant {\\n       Staker storage staker = stakers[msg.sender];\\n       // Stakers can only unstaked stake balance that is unlocked. Stake balance\\n       // becomes locked when it is used to underwrite a borrow.\\n       if (staker.stakedAmount - staker.locked < amount) revert InsufficientBalance();\\n       comptroller.withdrawRewards(msg.sender, stakingToken);\\n       uint256 remaining = IAssetManager(assetManager).withdraw(stakingToken, msg.sender, amount);\\n       if (uint96(remaining) > amount) {\\n           revert AssetManagerWithdrawFailed();\\n       }\\n       uint96 actualAmount = amount - uint96(remaining);\\n       _updateStakedCoinAge(msg.sender, staker);\\n       staker.stakedAmount -= actualAmount;\\n       totalStaked -= actualAmount;\\n       emit LogUnstake(msg.sender, actualAmount);\\n   }\\n```\\n\\n```\\ncontracts/user/UserManager.sol\\n\\n function _updateStakedCoinAge(address stakerAddress, Staker storage staker) private {\\n       uint64 currentBlock = uint64(block.number);\\n       uint256 lastWithdrawRewards = getLastWithdrawRewards[stakerAddress];\\n       uint256 blocksPast = (uint256(currentBlock) - _max(lastWithdrawRewards, uint256(staker.lastUpdated)));\\n       staker.stakedCoinAge += blocksPast * uint256(staker.stakedAmount);\\n       staker.lastUpdated = currentBlock;\\n   }\\n```\\n\\nAfter that Kiki calls the function `withdrawRewards` in order to withdraw his staking rewards. Everything executes fine, but the contract lacks union tokens and can't transfer the tokens to Kiki, so the else statement is triggered and the amount of tokens is added to his accrued balance, so he can still be able to withdraw them after.\\n```\\ncontracts/token/Comptroller.sol\\n\\n function withdrawRewards(address account, address token) external override whenNotPaused returns (uint256) {\\n       IUserManager userManager = _getUserManager(token);\\n       // Lookup account state from UserManager\\n       (UserManagerAccountState memory user, Info memory userInfo, uint256 pastBlocks) = _getUserInfo(\\n           userManager,\\n           account,\\n           token,\\n           0\\n       );\\n       // Lookup global state from UserManager\\n       uint256 globalTotalStaked = userManager.globalTotalStaked();\\n       uint256 amount = _calculateRewardsByBlocks(account, token, pastBlocks, userInfo, globalTotalStaked, user);\\n       // update the global states\\n       gInflationIndex = _getInflationIndexNew(globalTotalStaked, block.number - gLastUpdatedBlock);\\n       gLastUpdatedBlock = block.number;\\n       users[account][token].updatedBlock = block.number;\\n       users[account][token].inflationIndex = gInflationIndex;\\n       if (unionToken.balanceOf(address(this)) >= amount && amount > 0) {\\n           unionToken.safeTransfer(account, amount);\\n           users[account][token].accrued = 0;\\n           emit LogWithdrawRewards(account, amount);\\n           return amount;\\n       } else {\\n           users[account][token].accrued = amount;\\n           emit LogWithdrawRewards(account, 0);\\n           return 0;\\n       }\\n   }\\n```\\n\\nThis is where the issue occurs, next time Kiki calls the function `withdrawRewards`, he is going to lose all of his rewards.\\nExplanation of how this happens:\\nFirst the internal function _getUserInfo will return the struct `UserManagerAccountState memory user`, which contains zero amount for effectiveStaked, because Kiki unstaked all of his funds and already called the function withdrawRewards once. This happens because Kiki has `stakedAmount = 0, stakedCoinAge = 0, lockedCoinAge = 0, frozenCoinAge = 0`.\\n```\\n(UserManagerAccountState memory user, Info memory userInfo, uint256 pastBlocks) = _getUserInfo(\\n            userManager,\\n            account,\\n            token,\\n            0\\n        );\\n```\\n\\nThe cache `uint256 amount` will have a zero value because of the if statement applied in the internal function `_calculateRewardsByBlocks`, the if statement will be triggered as Kiki's effectiveStaked == 0, and as a result the function will return zero.\\n```\\nuint256 amount = _calculateRewardsByBlocks(account, token, pastBlocks, userInfo, globalTotalStaked, user);\\n```\\n\\n```\\nif (user.effectiveStaked == 0 || totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0) {\\n            return 0;\\n        }\\n```\\n\\nSince the cache `uint256 amount` have a zero value, the if statement in the function `withdrawRewards` will actually be ignored because of `&& amount > 0`. And the else statement will be triggered, which will override Kiki's accrued balance with "amount", which is actually zero. As a result Kiki will lose his rewards.\\n```\\nif (unionToken.balanceOf(address(this)) >= amount && amount > 0) {\\n            unionToken.safeTransfer(account, amount);\\n            users[account][token].accrued = 0;\\n            emit LogWithdrawRewards(account, amount);\\n\\n            return amount;\\n        } else {\\n            users[account][token].accrued = amount;\\n            emit LogWithdrawRewards(account, 0);\\n\\n            return 0;\\n        }\\n```\\n
One way of fixing this problem, that l can think of is to refactor the function _calculateRewardsByBlocks. First the function _calculateRewardsByBlocks will revert if `(totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0)`. Second new if statement is created, which is triggered if `user.effectiveStaked == 0`.\\nif `userInfo.accrued == 0`, it will return 0.\\nif `userInfo.accrued != 0`, it will return the accrued balance.\\n```\\nfunction _calculateRewardsByBlocks(\\n        address account,\\n        address token,\\n        uint256 pastBlocks,\\n        Info memory userInfo,\\n        uint256 totalStaked,\\n        UserManagerAccountState memory user\\n    ) internal view returns (uint256) {\\n        uint256 startInflationIndex = users[account][token].inflationIndex;\\n\\n        if (totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0) {\\n            revert ZeroNotAllowed();\\n        }\\n        \\n        if (user.effectiveStaked == 0) {\\n           if (userInfo.accrued == 0) return 0;\\n           else return userInfo.accrued\\n        }\\n\\n        uint256 rewardMultiplier = _getRewardsMultiplier(user);\\n\\n        uint256 curInflationIndex = _getInflationIndexNew(totalStaked, pastBlocks);\\n\\n        if (curInflationIndex < startInflationIndex) revert InflationIndexTooSmall();\\n\\n        return\\n            userInfo.accrued +\\n            (curInflationIndex - startInflationIndex).wadMul(user.effectiveStaked).wadMul(rewardMultiplier);\\n    }\\n```\\n
The impact here is that users can lose their staking rewards.\\nTo understand the scenario which is described in `Vulnerability Detail`, you'll need to know how the codebase works. Here in the impact section, l will describe in little more details and trace the functions.\\nThe issue occurs in 3 steps like described in Vulnerability Detail:\\nUser unstakes all of his funds.\\nThen he calls the function `withdrawRewards` in order to withdraw his rewards, everything executes fine but the contract lacks union tokens, so instead of transferring the tokens to the user, they are added to his accrued balance so he can still withdraw them after.\\nThe next time the user calls the function `withdrawRewards` in order to withdraw his accrued balance of tokens, he will lose all of his rewards.\\nExplanation in details:\\nUser unstakes all of his funds by calling the function `unstake`.\\nHis stakedAmount will be reduced to zero in the struct `Staker`.\\nHis stakedCoinAge will be updated to the current block with the internal function `_updateStakedCoinAge`.\\nThen he calls the function withdrawRewards in order to withdraw his rewards, everything executes fine but the contract lacks union tokens, so instead of transferring the tokens to the user, they are added to his accrued balance so he can still withdraw them after.\\nUser's stakedCoinAge, lockedCoinAge and frozenCoinAge are reduced to zero in the function `onWithdrawRewards`.\\nThe next time the user calls the function `withdrawRewards` in order to withdraw his accrued balance of tokens, he will lose all of his rewards.\\nIn order to withdraw his accrued rewards stored in his struct balance `Info`. He calls the function `withdrawRewards` again and this is where the issue occurs, as the user has `stakedAmount = 0, stakedCoinAge = 0, lockedCoinAge = 0, frozenCoinAge = 0` .\\nDuo to that the outcome of the function _getCoinAge, which returns a memory struct of CoinAge to the function `_getEffectiveAmounts` will look like this:\\n```\\nCoinAge memory coinAge = CoinAge({\\n            lastWithdrawRewards: lastWithdrawRewards,\\n            diff: diff,\\n            stakedCoinAge: staker.stakedCoinAge + diff * uint256(staker.stakedAmount),\\n            lockedCoinAge: staker.lockedCoinAge,\\n            frozenCoinAge: frozenCoinAge[stakerAddress]\\n        });\\n\\n// The function will return:\\nCoinAge memory coinAge = CoinAge({\\n            lastWithdrawRewards: random number,\\n            diff: random number,\\n            stakedCoinAge: 0 + random number * 0,\\n            lockedCoinAge: 0, \\n            frozenCoinAge: 0\\n        });\\n```\\n\\nAs a result the function `_getEffectiveAmounts` will return zero values for effectiveStaked and effectiveLocked to the function `onWithdrawRewards`.\\n```\\nreturn (\\n            // staker's total effective staked = (staked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (coinAge.stakedCoinAge - coinAge.frozenCoinAge) / coinAge.diff,\\n            // effective locked amount = (locked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (coinAge.lockedCoinAge - coinAge.frozenCoinAge) / coinAge.diff,\\n            memberTotalFrozen\\n        );\\n\\nreturn (\\n            // staker's total effective staked = (staked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (0 - 0) / random number,\\n             // effective locked amount = (locked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (0 - 0) / random number,\\n            0\\n        );\\n```\\n\\nAfter that the function `withdrawRewards` caches the returning value from the internal function `_calculateRewardsByBlocks`. What happens is that in the function `_calculateRewardsByBlocks` the if statement is triggered because the user's effectiveStaked == 0. As a result the internal function will return 0 and the cache `uint256 amount` will equal zero.\\n```\\nuint256 amount = _calculateRewardsByBlocks(account, token, pastBlocks, userInfo, globalTotalStaked, user);\\n```\\n\\n```\\nif (user.effectiveStaked == 0 || totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0) {\\n            return 0;\\n        }\\n```\\n\\nSince the cache `uint256 amount` have a zero value, the if statement in the function `withdrawRewards` will actually be ignored because of `&& amount > 0`. And the else statement will be triggered, which will override Kiki's accrued balance with "amount", which is actually zero.\\n```\\nif (unionToken.balanceOf(address(this)) >= amount && amount > 0) {\\n            unionToken.safeTransfer(account, amount);\\n            users[account][token].accrued = 0;\\n            emit LogWithdrawRewards(account, amount);\\n\\n            return amount;\\n        } else {\\n            users[account][token].accrued = amount;\\n            emit LogWithdrawRewards(account, 0);\\n\\n            return 0;\\n        }\\n```\\n\\nBelow you can see the functions which are invoked starting from the function _getUserInfo:\\n```\\n(UserManagerAccountState memory user, Info memory userInfo, uint256 pastBlocks) = _getUserInfo(\\n            userManager,\\n            account,\\n            token,\\n            0\\n        );\\n```\\n\\n```\\nfunction _getUserInfo(\\n        IUserManager userManager,\\n        address account,\\n        address token,\\n        uint256 futureBlocks\\n    ) internal returns (UserManagerAccountState memory user, Info memory userInfo, uint256 pastBlocks) {\\n        userInfo = users[account][token];\\n        uint256 lastUpdatedBlock = userInfo.updatedBlock;\\n        if (block.number < lastUpdatedBlock) {\\n            lastUpdatedBlock = block.number;\\n        }\\n\\n        pastBlocks = block.number - lastUpdatedBlock + futureBlocks;\\n\\n        (user.effectiveStaked, user.effectiveLocked, user.isMember) = userManager.onWithdrawRewards(\\n            account,\\n            pastBlocks\\n        );\\n    }\\n```\\n\\n```\\nfunction onWithdrawRewards(address staker, uint256 pastBlocks)\\n        external\\n        returns (\\n            uint256 effectiveStaked,\\n            uint256 effectiveLocked,\\n            bool isMember\\n        )\\n    {\\n        if (address(comptroller) != msg.sender) revert AuthFailed();\\n        uint256 memberTotalFrozen = 0;\\n        (effectiveStaked, effectiveLocked, memberTotalFrozen) = _getEffectiveAmounts(staker, pastBlocks);\\n        stakers[staker].stakedCoinAge = 0;\\n        stakers[staker].lastUpdated = uint64(block.number);\\n        stakers[staker].lockedCoinAge = 0;\\n        frozenCoinAge[staker] = 0;\\n        getLastWithdrawRewards[staker] = block.number;\\n\\n        uint256 memberFrozenBefore = memberFrozen[staker];\\n        if (memberFrozenBefore != memberTotalFrozen) {\\n            memberFrozen[staker] = memberTotalFrozen;\\n            totalFrozen = totalFrozen - memberFrozenBefore + memberTotalFrozen;\\n        }\\n\\n        isMember = stakers[staker].isMember;\\n    }\\n```\\n\\n```\\nfunction _getEffectiveAmounts(address stakerAddress, uint256 pastBlocks)\\n        private\\n        view\\n        returns (\\n            uint256,\\n            uint256,\\n            uint256\\n        )\\n    {\\n        uint256 memberTotalFrozen = 0;\\n        CoinAge memory coinAge = _getCoinAge(stakerAddress);\\n\\n        uint256 overdueBlocks = uToken.overdueBlocks();\\n        uint256 voucheesLength = vouchees[stakerAddress].length;\\n        // Loop through all of the stakers vouchees sum their total\\n        // locked balance and sum their total currDefaultFrozenCoinAge\\n        for (uint256 i = 0; i < voucheesLength; i++) {\\n            // Get the vouchee record and look up the borrowers voucher record\\n            // to get the locked amount and lastUpdated block number\\n            Vouchee memory vouchee = vouchees[stakerAddress][i];\\n            Vouch memory vouch = vouchers[vouchee.borrower][vouchee.voucherIndex];\\n\\n            uint256 lastRepay = uToken.getLastRepay(vouchee.borrower);\\n            uint256 repayDiff = block.number - _max(lastRepay, coinAge.lastWithdrawRewards);\\n            uint256 locked = uint256(vouch.locked);\\n\\n            if (overdueBlocks < repayDiff && (coinAge.lastWithdrawRewards != 0 || lastRepay != 0)) {\\n                memberTotalFrozen += locked;\\n                if (pastBlocks >= repayDiff) {\\n                    coinAge.frozenCoinAge += (locked * repayDiff);\\n                } else {\\n                    coinAge.frozenCoinAge += (locked * pastBlocks);\\n                }\\n            }\\n\\n            uint256 lastUpdateBlock = _max(coinAge.lastWithdrawRewards, uint256(vouch.lastUpdated));\\n            coinAge.lockedCoinAge += (block.number - lastUpdateBlock) * locked;\\n        }\\n\\n        return (\\n            // staker's total effective staked = (staked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (coinAge.stakedCoinAge - coinAge.frozenCoinAge) / coinAge.diff,\\n            // effective locked amount = (locked coinage - frozen coinage) / (# of blocks since last reward claiming)\\n            coinAge.diff == 0 ? 0 : (coinAge.lockedCoinAge - coinAge.frozenCoinAge) / coinAge.diff,\\n            memberTotalFrozen\\n        );\\n    }\\n```\\n\\n```\\nfunction _getCoinAge(address stakerAddress) private view returns (CoinAge memory) {\\n        Staker memory staker = stakers[stakerAddress];\\n\\n        uint256 lastWithdrawRewards = getLastWithdrawRewards[stakerAddress];\\n        uint256 diff = block.number - _max(lastWithdrawRewards, uint256(staker.lastUpdated));\\n\\n        CoinAge memory coinAge = CoinAge({\\n            lastWithdrawRewards: lastWithdrawRewards,\\n            diff: diff,\\n            stakedCoinAge: staker.stakedCoinAge + diff * uint256(staker.stakedAmount),\\n            lockedCoinAge: staker.lockedCoinAge,\\n            frozenCoinAge: frozenCoinAge[stakerAddress]\\n        });\\n\\n        return coinAge;\\n    }\\n```\\n\\nBelow you can see the function _calculateRewardsByBlocks:\\n```\\nfunction _calculateRewardsByBlocks(\\n        address account,\\n        address token,\\n        uint256 pastBlocks,\\n        Info memory userInfo,\\n        uint256 totalStaked,\\n        UserManagerAccountState memory user\\n    ) internal view returns (uint256) {\\n        uint256 startInflationIndex = users[account][token].inflationIndex;\\n\\n        if (user.effectiveStaked == 0 || totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0) {\\n            return 0;\\n        }\\n\\n        uint256 rewardMultiplier = _getRewardsMultiplier(user);\\n\\n        uint256 curInflationIndex = _getInflationIndexNew(totalStaked, pastBlocks);\\n\\n        if (curInflationIndex < startInflationIndex) revert InflationIndexTooSmall();\\n\\n        return\\n            userInfo.accrued +\\n            (curInflationIndex - startInflationIndex).wadMul(user.effectiveStaked).wadMul(rewardMultiplier);\\n    }\\n```\\n
```\\ncontracts/user/UserManager.sol\\n\\n function unstake(uint96 amount) external whenNotPaused nonReentrant {\\n       Staker storage staker = stakers[msg.sender];\\n       // Stakers can only unstaked stake balance that is unlocked. Stake balance\\n       // becomes locked when it is used to underwrite a borrow.\\n       if (staker.stakedAmount - staker.locked < amount) revert InsufficientBalance();\\n       comptroller.withdrawRewards(msg.sender, stakingToken);\\n       uint256 remaining = IAssetManager(assetManager).withdraw(stakingToken, msg.sender, amount);\\n       if (uint96(remaining) > amount) {\\n           revert AssetManagerWithdrawFailed();\\n       }\\n       uint96 actualAmount = amount - uint96(remaining);\\n       _updateStakedCoinAge(msg.sender, staker);\\n       staker.stakedAmount -= actualAmount;\\n       totalStaked -= actualAmount;\\n       emit LogUnstake(msg.sender, actualAmount);\\n   }\\n```\\n