Med: withdrawDETH is not functional for array lengths greater than one.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L87

Vulnerability details

Description

The withdrawDETH() function is used in GiantSavETHVaultPool to burn user's LP tokens and grant them dETH. It loops over all input vaults and all input LPTokens, and for each one calls lpTokenETH.burn(msg.sender, amount); Before that, it uses _assertUserHasEnoughGiantLPToClaimVaultLP check:

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");
}

It turns out this function will never be able to work unless user specifies only 1 LP token and 1 Vault. In any other case, this check will revert lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp .

lpTokenETH.burn() does:

function burn(address _recipient, uint256 _amount) external {
    require(msg.sender == pool, "Only pool");
    _burn(_recipient, _amount);
}

_burn calls transfer hooks:

function _burn(address account, uint256 amount) internal virtual {
    require(account != address(0), "ERC20: burn from the zero address");
    _beforeTokenTransfer(account, address(0), amount);
    uint256 accountBalance = _balances[account];
    require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
    unchecked {
        _balances[account] = accountBalance - amount;
    }
    _totalSupply -= amount;
    emit Transfer(account, address(0), amount);
    _afterTokenTransfer(account, address(0), amount);
}

The afterTokenTransferHook updates the timestamp of sender and receiver of GiantLP token:

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);
}

The end result is that after 1 transfer, the lastInteractedTimestamp of msg.sender will be current timestamp, and then it will be checked in the next iteration of withdrawDETH, which will assert that this value is greater than 1 day ago. That will cause revert.

Impact

withdrawDETH is not functional for array lengths greater than one.

Tools Used

Manual audit

Recommended Mitigation Steps

Use a different accounting technique to check recent use of lpTokenETH, so that withdrawals are allowed.

[c4-judge]

dmvt marked the issue as duplicate of #145

[c4-judge]

dmvt marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #382