User Tokens will be stuck forever

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L278-L287

Vulnerability details

Impact

User deposit will be stuck forever in the PrincipalToken contract if the contract need Approval before withdrawal from the Vault the contract (standard EIP4626)

Proof of Concept

According to the EIP4626 standard it's possible that some Vault may implement a pre-requesting withdraw or redeem check in the Vault before a withdrawal may be performed and PrincipalToken.sol contract doesn't have any implementation that can first request the token from the Vault, before the withdrawal process resulting the tokens will be stuck forever in the PrincipalToken contract and users will going to loose all of their tokens for this.

EIP4626 snippet for Withdrawal:

Note that some implementations will require pre-requesting to the Vault before a withdrawal may be performed. Those methods should be performed separately.

EIP4626 snippet for Redeem:

Note that some implementations will require pre-requesting to the Vault before a withdrawal may be performed. Those methods should be performed separately.

Demo Vault Code:

// SPDX-License-Identifier: Unlicensed

pragma solidity 0.8.20;

import "openzeppelin-erc20/ERC20Upgradeable.sol";
import "openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";
import "openzeppelin-contracts/interfaces/IERC4626.sol";
import "src/mocks/base/SpectraERC4626Upgradeable.sol";

contract VaultMock is ERC20Upgradeable, SpectraERC4626Upgradeable {
    using SafeERC20 for IERC20;

    uint256 private pricePerFullShare;
    uint256 private IBT_UNIT;
    mapping(address => bool) requestedWithdrawal;

    /**
     * @notice Initializer of the contract.
     * @param _name The name of the mock ibt token.
     * @param _symbol The symbol of the mock ibt token.
     * @param mockAsset The mock asset of the mock ibt token.
     */
    function initialize(
        string memory _name,
        string memory _symbol,
        IERC20Metadata mockAsset
    ) public initializer {
        __ERC20_init(_name, _symbol);
        __SpectraERC4626_init(mockAsset);
        pricePerFullShare = 1e18;
        IBT_UNIT = 10 ** mockAsset.decimals();
    }

    function requestWithdraw(address owner) public {
        requestedWithdrawal[owner] = true;
    }

    /**
     * @notice Function to update the price of IBT to its underlying token.
     * @param _price The new price of the ibt.
     */
    function setPricePerFullShare(uint256 _price) public {
        pricePerFullShare = _price;
    }

    /**
     * @notice Function to convert the no of shares to it's amount in assets.
     * @param shares The no of shares to convert.
     * @return The amount of assets from the specified shares.
     */
    function convertToAssets(uint256 shares) public view override returns (uint256) {
        return (shares * pricePerFullShare) / IBT_UNIT;
    }

    /**
     * @notice Function to convert the no of assets to it's amount in shares.
     * @param assets The no of assets to convert.
     * @return The amount of shares from the specified assets.
     */
    function convertToShares(uint256 assets) public view override returns (uint256) {
        if (pricePerFullShare == 0) {
            return 0;
        }
        return (assets * IBT_UNIT) / pricePerFullShare;
    }

    /**
     * @notice Function to deposit the provided amount in assets.
     * @param amount The amount of assets to deposit.
     * @param receiver The address of the receiver.
     * @return shares The amount of shares received.
     */
    function deposit(uint256 amount, address receiver) public override returns (uint256 shares) {
        IERC20(address(_asset)).safeTransferFrom(msg.sender, address(this), amount);
        shares = convertToShares(amount);
        _mint(receiver, shares);
    }

    /**
     * @notice Function to withdraw the provided no of shares.
     * @param assets The amount of assets to withdraw.
     * @param receiver The address of the receiver.
     * @return shares The amount of shares to burn to withdraw assets.
     */
    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override(IERC4626) returns (uint256 shares) {
        require(requestedWithdrawal[owner], "Should request First");
        shares = convertToShares(assets);
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }
        _burn(owner, shares);
        IERC20(address(_asset)).safeTransfer(receiver, assets);
        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

    /** @dev See {IERC4626-previewDeposit}. */
    function previewDeposit(uint256 _amount) public view override returns (uint256) {
        return convertToShares(_amount);
    }

    /** @dev See {IERC4626-previewWithdraw}. */
    function previewWithdraw(uint256 assets) public view override returns (uint256) {
        return convertToShares(assets);
    }

    /** @dev See {IERC4626-previewRedeem}. */
    function previewRedeem(uint256 shares) public view virtual override returns (uint256) {
        return convertToAssets(shares);
    }

    /** @dev See {IERC4626-redeem}. */
    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {
        require(requestedWithdrawal[owner], "Should request First");
        if (shares > maxRedeem(owner)) {
            revert SharesMoreThanMaxValue();
        }
        uint256 assets = previewRedeem(shares);

        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }
        _burn(owner, shares);
        IERC20(address(_asset)).safeTransfer(receiver, assets);
        emit Withdraw(msg.sender, receiver, owner, assets, shares);
        return assets;
    }
}

PoC to Test revert:

function testApprovalWithdraw() external {
        uint256 amountToDeposit = 1e18;
        _prepareForDepositIBT(testUser, amountToDeposit);
        console.log(ibt.convertToAssets(10 ** ibt.decimals()));
        uint eb1 = underlying.balanceOf(testUser);
        console.log("Withdraw Balance:", eb1);
        vm.startPrank(testUser);
        ibt.approve(address(principalToken), amountToDeposit);

        // initial deposit
        principalToken.depositIBT(amountToDeposit, testUser);
        vm.stopPrank();

        vm.prank(testUser);
        vm.expectRevert();
        principalToken.redeem(0, testUser, testUser);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Implement a function which can be used to call approval function of the Tokenized Vault contract(ERC4626) for the Vaults which required a approval before withdraw/redeem.

Assessed type

DoS

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as high quality report

[c4-sponsor]

yanisepfl (sponsor) disputed

[c4-sponsor]

yanisepfl marked the issue as disagree with severity

[yanisepfl]

All our main functions (e.g. deposit / withdraw / redeem / claim) have an associated method that allows users to directly interact with our protocol using/receiving IBTs. Hence the claim that "User Tokens will be stuck forever" is false.

Secondly, in the mentioned situation, the mitigation proposed would not be working if for instance the pre-request is to wait a certain period of time (e.g. wait 24 hours to withdraw).

We therefore dispute this issue and disagree with its severity. We consider it as a QA report. As a low issue, we acknowledge it but we will not be mitigating it since no user funds are at risk and there is not much we can do for such particular 4626 vaults.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid