deny others(griefing) from calling withdrawLPTokens() in GiantPoolBase and withdrawing LPtoken by sending some token to them(reset lastInteractedTimestamp)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L43-L47 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L66-L97

Vulnerability details

Impact

Attacker can perform DOS and prevent users from calling some functions of giant pools that checks lastInteractedTimestamp. functions like withdrawLPTokens() which checks lastInteractedTimestamp of user and make user user only interact 1 time every day is vulnerable to this attack because when transfer of LPToken happens contract set lastInteractedTimestamp for _from and _to and attacker can send small amount of LPToken to target users and reset their lastInteractedTimestamp and cause DOS. also attacker can front-run user's transaction and perform this attack and make user's transaction to fail and cause griefing.

Proof of Concept

This is withdrawLPTokens() code in GiantPoolBase (which is base class for Giant Pools):

    /// @notice Allow a user to chose to withdraw vault LP tokens by burning their giant LP tokens. 1 Giant LP == 1 vault LP
    /// @param _lpTokens List of LP tokens being owned and being withdrawn from the giant pool
    /// @param _amounts List of amounts of giant LP being burnt in exchange for vault LP
    function withdrawLPTokens(LPToken[] calldata _lpTokens, uint256[] calldata _amounts) external {
        uint256 amountOfTokens = _lpTokens.length;
        require(amountOfTokens > 0, "Empty arrays");
        require(amountOfTokens == _amounts.length, "Inconsistent array lengths");

        _onWithdraw(_lpTokens);

        for (uint256 i; i < amountOfTokens; ++i) {
            LPToken token = _lpTokens[i];
            uint256 amount = _amounts[i];

            _assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

            // Burn giant LP from user before sending them an LP token from this pool
            lpTokenETH.burn(msg.sender, amount);

            // Giant LP tokens in this pool are 1:1 exchangeable with external savETH vault LP
            token.transfer(msg.sender, amount);

            emit LPSwappedForVaultLP(address(token), msg.sender, amount);
        }
    }

As you can see it calls _assertUserHasEnoughGiantLPToClaimVaultLP() to perform some checks and This is _assertUserHasEnoughGiantLPToClaimVaultLP() code:

    /// @dev Check the msg.sender has enough giant LP to burn and that the pool has enough savETH vault LP
    function _assertUserHasEnoughGiantLPToClaimVaultLP(LPToken _token, uint256 _amount) internal view {
        require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
        require(_token.balanceOf(address(this)) >= _amount, "Pool does not own specified LP");
        require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");
    }

As you can see it checks the lastInteractedTimestamp of msg.sender and makes sure that 1 day has passed from last interaction time. but lastInteractedTimestamp of users would be reset in LPToken transfers too, This is _afterTokenTransfer() code in GiantLP (LPToken is similar) :

    function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
        lastInteractedTimestamp[_from] = block.timestamp;
        lastInteractedTimestamp[_to] = block.timestamp;
        if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
    }

As you can see it reset lastInteractedTimestamp of _from and _to address and whenever a transfer happens lastInteractedTimestamp of _from and _to address would be reset. so to perform this attack, attacker can perform this steps:

1. send small amount of LPToken to user address.
2. contract reset lastInteractedTimestamp of user.
3. user can't call function like withdrawLPTokens() which checks user lastInteractedTimestamp. Because attacker only need to perform on transaction each day so it's possible for attacker to continue doing this with small cost.

Another way to perform this attack is by front-running user transaction and send small number of LPToken to user and reset user lastInteractedTimestamp and cause user transaction to fail(cause griefing). to perform this attacker can perform this steps:

1. wait for user transaction which calls withdrawLPTokens() or other function which checks lastInteractedTimestamp.
2. front-run user transaction and transfer small amount of LPToken to the user so lastInteractedTimestamp of user would be reset.
3. user transaction would fail because of lastInteractedTimestamp check. This second scenario can be performed to burnLPToken() of SavETHVault() and burnLPForETH(), claimRewards() of StakingFundsVault because they check for lastInteractedTimestamp too and user only interact with this functions every 30 minutes, so attacker can perform front-running for this functions too. In this scenario attacker perform DOS and griefing. user can't call those functions and manage his funds and user pays gas fee for failed transactions.

Tools Used

VIM

Recommended Mitigation Steps

don't reset lastInteractedTimestamp for _to in transactions.

[c4-judge]

dmvt marked the issue as duplicate of #59

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as duplicate of #49

[c4-judge]

dmvt marked the issue as satisfactory