code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1623

Vulnerability details

Impact

Altering how reward points are calculated for LP staking, could cause other sets of stakers to suffer sizable financial loses, and it might cause the rewards reserve of the pool to run out and lessen motivation for LP staking.

Proof of Concept

By changing the value of the _DIVISOR property to zero, an adversary can take advantage of the _withdrawLP() function flaw. When the code tries to compute "points = amount 100 / 1e18 lpPosition.multiplier / _DIVISOR", a division by zero error will result.

The _withdrawLP() method can be changed to conduct a check for _DIVISOR before executing the calculation in order to show this vulnerability. Here is the changed function:

function _withdrawLP () private {
    uint256 amount;
    assembly{
        amount := calldataload(0x24)
    }

    // Validate that the caller has cleared their asset timelock.
    LPPosition storage lpPosition = stakerLPPosition[msg.sender];
    if (block.timestamp < lpPosition.timelockEndTime) {
        revert TimelockNotCleared(lpPosition.timelockEndTime);
    }

    // Validate that the caller has enough staked LP tokens to withdraw.
    if (lpPosition.amount < amount) {
        revert NotEnoughLPTokens(amount, lpPosition.amount);
    }

    /*
        Attempt to transfer the LP tokens held in escrow by this staking contract 
        back to the caller.
    */
    _assetTransfer(LP, msg.sender, amount);

    // Update caller staking information and asset data.
    PoolData storage pool = _pools[AssetType.LP];
    unchecked {
        uint256 points;
+       if (_DIVISOR == 0) {
            points = 0;
        } else {
            points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
        }
        // Update the caller's LP token stake.
        lpPosition.amount -= amount;
        lpPosition.points -= points;

        // Update the pool point weights for rewards.
        pool.totalPoints -= points;
    }

    // If all LP tokens are withdrawn, we must clear the multiplier.
    if (lpPosition.amount == 0) {
        lpPosition.multiplier = 0;
    }

    // Emit an event recording this LP withdraw.
    emit Withdraw(
        msg.sender,
        LP,
        amount
    );
}

A malicious contract that uses the _withdrawLP() method and sets _DIVISOR to zero can be used by an adversary to take advantage of the flaw.

Tools Used

Manual audit

Recommended Mitigation Steps

Add a check for the value of _DIVISOR before dividing, the check can be done using the require statement to ensure the _DIVISOR value is greater than zero.

Affected code: https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1597-L1644

hansfriese commented 1 year ago

_DIVISOR is a constant 100.

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-03-neotokyo-findings