It's possible to call migrate() function of BkdLocker with newRewardToken value equal to current rewardToken and there is no check to prevent this. and if this happens then _userCheckpoint() will calculated reward double times for rewardToken, one time because of it's the current reward token and one time because it's in _replacedRewardTokens list and this is gonna make some users lose their rewards.
Proof of Concept
This is migrate() code in BkdLocker:
/**
* @notice Sets a new token to be the rewardToken. Fees are then accumulated in this token.
* @dev Previously used rewardTokens can be set again here.
*/
function migrate(address newRewardToken) external override onlyGovernance {
_replacedRewardTokens.remove(newRewardToken);
_replacedRewardTokens.set(rewardToken, block.timestamp);
lastMigrationEvent = block.timestamp;
rewardToken = newRewardToken;
}
As you can see there is no check that prevent execution when newRewardToken == rewardToken, so if by mistake or by bad intention migrate is called with newRewardToken == rewardToken, then code will add newRewardToken to current rewardToken and in _replacedRewardTokens list too. so in the _userCheckpoint() contract is going to calculate rewards double time for rewardToken. This is _userCheckpoint() code:
function _userCheckpoint(
address user,
uint256 amountAdded,
uint256 newTotal
) internal {
RewardTokenData storage curRewardTokenData = rewardTokenData[rewardToken];
// Compute the share earned by the user since they last updated
uint256 userBalance = balances[user];
if (userBalance > 0) {
curRewardTokenData.userShares[user] += (curRewardTokenData.feeIntegral -
curRewardTokenData.userFeeIntegrals[user]).scaledMul(
userBalance.scaledMul(boostFactors[user])
);
// Update values for previous rewardTokens
if (lastUpdated[user] < lastMigrationEvent) {
uint256 length = _replacedRewardTokens.length();
for (uint256 i; i < length; i = i.uncheckedInc()) {
(address token, uint256 replacedAt) = _replacedRewardTokens.at(i);
if (lastUpdated[user] < replacedAt) {
RewardTokenData storage prevRewardTokenData = rewardTokenData[token];
prevRewardTokenData.userShares[user] += (prevRewardTokenData.feeIntegral -
prevRewardTokenData.userFeeIntegrals[user]).scaledMul(
userBalance.scaledMul(boostFactors[user])
);
prevRewardTokenData.userFeeIntegrals[user] = prevRewardTokenData
.feeIntegral;
}
}
}
}
uint256 newBoost = computeNewBoost(user, amountAdded, newTotal);
totalLockedBoosted =
totalLockedBoosted +
newTotal.scaledMul(newBoost) -
balances[user].scaledMul(boostFactors[user]);
// Update user values
curRewardTokenData.userFeeIntegrals[user] = curRewardTokenData.feeIntegral;
lastUpdated[user] = block.timestamp;
boostFactors[user] = newBoost;
balances[user] = newTotal;
}
As you can see first it set curRewardTokenData = rewardTokenData[rewardToken] so curRewardTokenData is showing user rewards in rewardToken and then it calculates curRewardTokenData.userShares[user]. it's updating its value by adding new reward tokens to it. after that it loops through _replacedRewardTokens list and it sets prevRewardTokenData = rewardTokenData[token] and then updates prevRewardTokenData.userShares[user] value by adding new reward tokens. and because rewardToken is in _replacedRewardTokens too and code uses += to update values so new rewards is going to be added to rewardTokenData[rewardToken].userShares[user] double times.
One more thing is that to update previous rewards token data code uses += and prevRewardTokenData.userFeeIntegrals[user] but for rewardToken in _replacedRewardTokens the value of prevRewardTokenData.userFeeIntegrals[user] is not updated in first part (when calculating current reward token for user) and curRewardTokenData.userFeeIntegrals[user] is updated in last part of function's code in line 332:
So the logic will add new reward token for user double time if current rewardToken is in _replacedRewardTokens too. The value of rewardTokenData[rewardToken].userShares[user] will be wrong number and if some users call claimFees(rewardToken) which didn't updated his rewards for some time (his checkpoint timestamp is older than wrong migrate() call) then his rewards is going to be calculated double time and he is going to receive other users rewards.
This bug will easily make users lose funds and contract has no protection in migrate to prevent it and the logics of migrate is not compelete.
Tools Used
VIM
Recommended Mitigation Steps
in migrate() check that new reward token is not equal to old reward token.
Lines of code
https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L66-L75 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L292-L336
Vulnerability details
Impact
It's possible to call
migrate()
function ofBkdLocker
withnewRewardToken
value equal to currentrewardToken
and there is no check to prevent this. and if this happens then_userCheckpoint()
will calculated reward double times forrewardToken
, one time because of it's the current reward token and one time because it's in_replacedRewardTokens
list and this is gonna make some users lose their rewards.Proof of Concept
This is
migrate()
code inBkdLocker
:As you can see there is no check that prevent execution when
newRewardToken == rewardToken
, so if by mistake or by bad intentionmigrate
is called withnewRewardToken == rewardToken
, then code will addnewRewardToken
to currentrewardToken
and in_replacedRewardTokens
list too. so in the_userCheckpoint()
contract is going to calculate rewards double time forrewardToken
. This is_userCheckpoint()
code:As you can see first it set
curRewardTokenData = rewardTokenData[rewardToken]
socurRewardTokenData
is showing user rewards inrewardToken
and then it calculatescurRewardTokenData.userShares[user]
. it's updating its value by adding new reward tokens to it. after that it loops through_replacedRewardTokens
list and it setsprevRewardTokenData = rewardTokenData[token]
and then updatesprevRewardTokenData.userShares[user]
value by adding new reward tokens. and becauserewardToken
is in_replacedRewardTokens
too and code uses+=
to update values so new rewards is going to be added torewardTokenData[rewardToken].userShares[user]
double times. One more thing is that to update previous rewards token data code uses+=
andprevRewardTokenData.userFeeIntegrals[user]
but forrewardToken
in_replacedRewardTokens
the value ofprevRewardTokenData.userFeeIntegrals[user]
is not updated in first part (when calculating current reward token for user) andcurRewardTokenData.userFeeIntegrals[user]
is updated in last part of function's code in line 332:So the logic will add new reward token for user double time if current
rewardToken
is in_replacedRewardTokens
too. The value ofrewardTokenData[rewardToken].userShares[user]
will be wrong number and if some users callclaimFees(rewardToken)
which didn't updated his rewards for some time (his checkpoint timestamp is older than wrongmigrate()
call) then his rewards is going to be calculated double time and he is going to receive other users rewards. This bug will easily make users lose funds and contract has no protection inmigrate
to prevent it and the logics ofmigrate
is not compelete.Tools Used
VIM
Recommended Mitigation Steps
in
migrate()
check that new reward token is not equal to old reward token.