Potential DDoS for S1Citizen to re-stake BYTES

[code423n4]

Lines of code

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

Vulnerability details

Potential DDoS for S1Citizen to re-stake BYTES

Files:

1. NeoTokyoStaker.sol - L926

   Description:

   The _stakeS1Citizen function in the NeoTokyoStaker smart contract allows a user to stake their S1Citizen NFT into an associated vault. However, there is no check placed to confirm the vaultID while staking the S1Citizen. Therefore, another user can stake their S1Citizen NFT by providing an unowned vaultID, which can lead to a the following attack scenario.

   Steps to reproduce:

   • User A creates an associated vault and stakes their S1Citizen token.
   • User B calls the _stakeS1Citizen function to stake their S1Citizen token to User A’s associated vault with user A’s vaultId

     Vulnerable Function:

     function _stakeS1Citizen (
     uint256 _timelock
     ) private {
     uint256 citizenId;
     uint256 vaultId;
     uint256 handClaimant;
     /*
     Extract the S1 Citizen ID, optional Vault token ID, and optional Hand
     claimant ID from calldata.
     */
     assembly {
     citizenId := calldataload(0x44)
     vaultId := calldataload(0x64)
     handClaimant := calldataload(0x84)
     }
     /*
     Attempt to transfer the S1 Citizen to be held in escrow by this staking
     contract. This transfer will fail if the caller is not the holder of the
     Citizen. This prevents double staking.
     */
     _assetTransferFrom(S1_CITIZEN, msg.sender, address(this), citizenId);
     // Retrieve storage for tracking the staking state of this S1 Citizen.
     StakedS1Citizen storage citizenStatus = stakedS1[msg.sender][citizenId];
     // Attach a getter to the S1 Citizen and check for a component Vault.
     IGenericGetter citizen = IGenericGetter(S1_CITIZEN);
     uint256 citizenVaultId = citizen.getVaultIdOfTokenId(citizenId);
     /*
     A new Vault to stake may only be provided if the S1 Citizen being staked
     does not already have a component Vault.
     */
     if (citizenVaultId != 0 && vaultId != 0) {
     revert CitizenAlreadyHasVault(citizenVaultId, vaultId);
     /*
     If no optional vault is provided, and the S1 Citizen being staked already
     has an existing Vault, override the provided `vaultId`.
     */
     } else if (citizenVaultId != 0 && vaultId == 0) {
     citizenStatus.hasVault = true;
     vaultId = citizenVaultId;
     /*
     Otherwise, if the S1 Citizen has no component Vault, the newly-provided
     Vault is staked and the S1 Citizen is recorded as carrying an optional,
     separately-attached vault.
     */
     } else if (citizenVaultId == 0 && vaultId != 0) {
     _assetTransferFrom(VAULT, msg.sender, address(this), vaultId);
     citizenStatus.hasVault = true;
     citizenStatus.stakedVaultId = vaultId;
     }
     /*
     If the S1 Citizen contains no component Vault and is not staked alongside
     an optional Vault (`citizenVaultId` == 0 && `vaultId` == 0), we need not
     do anything to change the initial state of a staked S1 Citizen's Vault.
     */
     // Determine the base worth in points of the S1 Citizen's Identity.
     string memory citizenCreditYield = getCreditYield(
     citizenId,
     citizenVaultId
     );
     uint256 identityPoints = identityCreditPoints[citizenCreditYield];
     // Hands of the Citadel are always given the same multiplier as '?' Vaults.
     uint256 vaultMultiplier = 100;
     if (handClaimant == 1) {
     uint256 identityId = citizen.getIdentityIdOfTokenId(citizenId);
     string memory class = IGenericGetter(IDENTITY).getClass(identityId);
     if (_stringEquals(class, "Hand of Citadel")) {
      vaultMultiplier = vaultCreditMultiplier["?"];
     } else {
      revert CitizenIsNotHand(citizenId);
     }
     // Otherwise use the configured Vault multiplier, if any.
     } else if (vaultId != 0) {
     vaultMultiplier = getConfiguredVaultMultiplier(vaultId);
     }
     // Decode the timelock option's duration and multiplier.
     uint256 timelockDuration = _timelock >> 128;
     uint256 timelockMultiplier = _timelock & type(uint128).max;
     // Update caller staking information and asset data.
     PoolData storage pool = _pools[AssetType.S1_CITIZEN];
     unchecked {
     citizenStatus.points =
      identityPoints * vaultMultiplier * timelockMultiplier /
      _DIVISOR / _DIVISOR;
     citizenStatus.timelockEndTime = block.timestamp + timelockDuration;
     // Record the caller's staked S1 Citizen.
     _stakerS1Position[msg.sender].push(citizenId);
     // Update the pool point weights for rewards
     pool.totalPoints += citizenStatus.points;
     }
     // Emit an event recording this S1 Citizen staking.
     emit Stake(
     msg.sender,
     S1_CITIZEN,
     _timelock,
     citizenId
     );
     }

     Impact:

     The vulnerability allows an attacker to stake their S1 citizen token into another user’s associated vault, which can lead to various impacts such as exceeding the maximum cap of the vault or the possibility of denial of service (DoS) attacks. Additionally, the attacker can withdraw all the rewards and points associated with the vault, causing significant financial losses to the victim.

     Recommendation:

     To address this vulnerability, a check should be implemented for the function argument vaultID while staking an S1Citizen to ensure that only the owner of the associated vault can stake their token to the respective vaultID.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid