Missed ERC20.approve() to PrincipalToken in PrincipalToken.deposit()

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

When user tries to deposit transaction will be reverted, if he hadn't approve token to PrincipalToken proxy address earlier. This is incorrect logic, ERC20 unlike ERC721 can be approved in the same transaction with transfer. As default best practice is to use approve in the same function, so some users' will lose funds on gas in first reverted transaction.

Proof of Concept

add this snippet to test/PrincipalToken/PrincipalToken.t.sol

function testDepositWithoutApprove() public {
        uint256 amountToDeposit = 1e18;

        // transfer must be reverted. deposit function missing approve() of erc20
        vm.expectRevert();
        principalToken.deposit(amountToDeposit, address(this));
}

Tools Used

Manual review, foundry

Recommended Mitigation Steps

add to deposit: IERC20(_asset).approve(address(this), assets);

function deposit(uint256 assets, address ptReceiver, address ytReceiver) public override returns (uint256 shares) {
        IERC20(_asset).approve(address(this), assets);
        IERC20(_asset).safeTransferFrom(msg.sender, address(this), assets);
        IERC20(_asset).safeIncreaseAllowance(ibt, assets);
        uint256 ibts = IERC4626(ibt).deposit(assets, address(this));
        shares = _depositIBT(ibts, ptReceiver, ytReceiver);
}

Assessed type

Token-Transfer

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[gzeon-c4]

safeIncreaseAllowance used

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid