_assetTransferFrom & _assetTransfer do not follow check-effect-interaction.

[code423n4]

Lines of code

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

Vulnerability details

Impact

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the checks-effects-interactions pattern.

Consider always moving the state-changes before the external calls.

I found the 2 specifc issues with funcation _stakeS1Citizen() and _withdrawLP() in NeoTokyoStaker.sol

Proof of Concept

    function _stakeS1Citizen (
        uint256 _timelock
    ) private {
...
        */
        } else if (citizenVaultId == 0 && vaultId != 0) {
            _assetTransferFrom(VAULT, msg.sender, address(this), vaultId); //@audit transfer before update variables
            citizenStatus.hasVault = true;   //update variable
            citizenStatus.stakedVaultId = vaultId;  //update variable

    function _withdrawLP () private {
        uint256 amount;
...
        }

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

        // Update caller staking information and asset data.
        PoolData storage pool = _pools[AssetType.LP];
        unchecked {
            uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

            // Update the caller's LP token stake.
            lpPosition.amount -= amount;   //update variable
            lpPosition.points -= points;   //update variable

Tools Used

Manual

Recommended Mitigation Steps

Follow the check-effect-interaction pattern.

[hansfriese]

Will keep for the sponsor's review. Looks like a low risk.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)

[c4-judge]

hansfriese marked the issue as primary issue

[TimTinkers]

There is no attack present here: every use of these functions is already wrapped in a reentrancy guard.

[c4-sponsor]

TimTinkers marked the issue as sponsor disputed

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid