Misconfiguration of LP token contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

If the LP token contract is set to a non-contract address or a no-revert-on-transfer token, users will be able to:

1. Mint huge amounts of BYTES 2.0 tokens.
2. Drain the contract of all its LP tokens.

Vulnerability Details

Throughout the contract, it is assumed that _assetTransferFrom() will revert if a user does not have sufficient balance. For example:

NeoTokyoStaker.sol#L1132-L1137

/*
    Attempt to transfer the LP tokens to be held in escrow by this staking 
    contract. This transfer will fail if the caller does not hold enough 
    tokens.
*/
_assetTransferFrom(LP, msg.sender, address(this), amount);

Due to this assumption, staking functions do not check for a user's balance, but rely on _assetTransferFrom() to revert if a user attempts to stake more tokens than he has.

The code of _assetTransferFrom() is as shown:

NeoTokyoStaker.sol#L796-L814

function _assetTransferFrom (
    address _asset,
    address _from,
    address _to,
    uint256 _idOrAmount
) private {
    (bool success, bytes memory data) = 
        _asset.call(
            abi.encodeWithSelector(
                _TRANSFER_FROM_SELECTOR,
                _from,
                _to, 
                _idOrAmount
            )
        );

    // Revert if the low-level call fails.
    if (!success) {
        revert(string(data));
    }
}

First, it uses a low-level call to call transferFrom() in the contract at _asset. Then, it checks success and reverts if the low-level call has failed.

This implementation allows _assertTransferFrom() to return silently, even if _from does not have sufficient balance, if one of the following is true:

1. _asset does not contain a contract.

• Low-level calls will always return true if the underlying address does not contain any code.
• Therefore, if the address in _asset does not contain a smart contract, success will always be true and _assetTransferFrom() will return without performing any token transfer.

2. The contract at _asset is a no-revert-on-transfer token.

• Some ERC20 tokens return false instead of reverting when a transfer fails. An example of such a token is ZRX.
• In _assetTransferFrom(), the return value from the call to transferFrom() is stored in data. However, this return value is not checked at all.
• Therefore, if _asset is set to a no-revert-on-transfer token, the function will not revert when transferFrom() returns false due to insufficient balance.

As the LP token contract can be set using configureLP(), it is possible for the LP contract address to fulfil one of the above. This would allow users to:

1. Mint huge amounts of BYTES 2.0 tokens.
2. Drain the contract of all its LP tokens.

Proof of Concept

First, Bob calls _stakeLP() with amount = type(uint256).max.

1. Even though Bob clearly does not have enough balance, the following call to assetTransferFrom() will not revert:

NeoTokyoStaker.sol#L1132-L1137

/*
    Attempt to transfer the LP tokens to be held in escrow by this staking 
    contract. This transfer will fail if the caller does not hold enough 
    tokens.
*/
_assetTransferFrom(LP, msg.sender, address(this), amount);

2. As such, Bob will gain a huge amount of points as amount directly influences the calculation of points:

NeoTokyoStaker.sol#L1155

uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

3. _stakeLP() then updates Bob's total points and adds type(uint256).max to his staked amount of LP tokens.

NeoTokyoStaker.sol#L1160-L1161

stakerLPPosition[msg.sender].amount += amount;
stakerLPPosition[msg.sender].points += points;

As Bob now has a huge amount of points, he calls getReward() to mint a huge amount of BYTES 2.0 tokens.

After Bob's timelock for LP token staking ends, he calls _withdrawLP with amount set to the contract's LP token balance.

1. As Bob's amount of LP tokens staked is stored as type(uint256).max, the following check passes:

NeoTokyoStaker.sol#L1609-L1612

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

2. All the LP tokens in the contract are then transferred to Bob. Note that this does not work if _asset is set to a non-contract address.

NeoTokyoStaker.sol#L1618

_assetTransfer(LP, msg.sender, amount);

Recommended Mitigation

There are two issues that need to be fixed:

1. _asset does not contain a contract.
2. The contract at _asset is a no-revert-on-transfer token.

To fix issue 1, in configureLP(), consider checking that the _lp address contains a contract.

To fix issue 2, ensure that the return value is true if there is return data from the low-level call. This is how _callOptionalReturnBool() in OpenZeppelin's SafeERC20 is implemented.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #78

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope