Backd Tokenomics contest - unforgiven's results

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 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:

        // Update user values
        curRewardTokenData.userFeeIntegrals[user] = curRewardTokenData.feeIntegral;
        lastUpdated[user] = block.timestamp;
        boostFactors[user] = newBoost;
        balances[user] = newTotal;
    }

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#L53-L64

Vulnerability details

Impact

function initialize() of BkdLocker suppose to be called one time and contract initialize one time. but if it's called by startBoost=0 then it's possible to call it again with different values for other parameters. there are some logics based on the values function initilize() sets which is in calculating boost and withdraw delay. by initializing multiple times different users get different values for those logics and because rewards are distributed based on boosts so those logics will be wrong too.

Proof of Concept

This is initiliaze() code in BkdLocker:

    function initialize(
        uint256 startBoost,
        uint256 maxBoost,
        uint256 increasePeriod,
        uint256 withdrawDelay
    ) external override onlyGovernance {
        require(currentUInts256[_START_BOOST] == 0, Error.CONTRACT_INITIALIZED);
        _setConfig(_START_BOOST, startBoost);
        _setConfig(_MAX_BOOST, maxBoost);
        _setConfig(_INCREASE_PERIOD, increasePeriod);
        _setConfig(_WITHDRAW_DELAY, withdrawDelay);
    }

As you can see it checks the initialization statue by currentUInts256[_START_BOOST]'s value but it's not correct way to do and initializer can set currentUInts256[_START_BOOST] value as 0 and set other parameters values and call this function multiple times with different values for _MAX_BOOST and _INCREASE_PERIOD and _WITHDRAW_DELAY. setting different values for those parameters can cause different calculation in computeNewBoost() and prepareUnlock(). function computeNewBoost() is used to calculate users boost parameters which is used on reward distribution. so by changing _MAX_BOOST the rewards will be distributed wrongly between old users and new users.

Tools Used

VIM

Recommended Mitigation Steps

add some other variable to check the status of initialization of contract.

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L227-L232

Vulnerability details

Impact

function lockFor() in BkdLocker is supposed to lock 'msg.senderfunds and increaseuseraddress funds but if anyone one calls it with0x0` address by mistake then his funds will be locked forever.

Proof of Concept

This is lockFor() code in BkdLocker:

    function lockFor(address user, uint256 amount) public override {
        govToken.safeTransferFrom(msg.sender, address(this), amount);
        _userCheckpoint(user, amount, balances[user] + amount);
        totalLocked += amount;
        emit Locked(user, amount);
    }

As you can see there is no check that user is not 0x0. code calls _userCheckpoint() which will increase 0x0 balances in the contract and there is no check in _userCheckpoint() either and user can lose all his funds just by one simple mistake.

Tools Used

VIM

Recommended Mitigation Steps

check that user is not 0x0 in lcokFor