code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

Improper Asset Transfer in Withdrawal Process #19

Closed c4-bot-4 closed 8 months ago

c4-bot-4 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L303-L313

Vulnerability details

Impact

The impact of this finding is that the withdrawIBT function in the contract has a potential issue in the final step where IBTs are transferred to the receiver. Instead of transferring an amount in assets, it directly transfers the ibts quantity, potentially leading to incorrect transfer values.

Proof of Concept

Here is the relevant code snippet from the withdrawIBT function:

function withdrawIBT(
    uint256 ibts,
    address receiver,
    address owner
) public override returns (uint256 shares) {
    _beforeWithdraw(IERC4626(ibt).previewRedeem(ibts), owner);
    (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
    shares = _withdrawShares(ibts, receiver, owner, _ptRate, _ibtRate);
    // send IBTs from this contract to receiver
    IERC20(ibt).safeTransfer(receiver, ibts);
}

The issue lies in the last line where ibts is directly transferred to the receiver without undergoing a proper conversion to assets.

Tools Used

Manual code review was performed.

Recommended Mitigation Steps

To address this issue, it is recommended to ensure that the ibts quantity is appropriately converted to assets before transferring to the receiver. The conversion logic should align with the intended value of the assets being transferred. This can be achieved by utilizing the proper conversion rates or methods to determine the equivalent asset value.

A possible modification to the withdrawIBT function could be:

uint256 assetValue = IERC4626(ibt).previewRedeem(ibts);
// ... other existing logic
IERC20(ibt).safeTransfer(receiver, assetValue);

By incorporating the calculated assetValue in the transfer, the contract ensures that the correct asset amount is transferred to the receiver.

This modification helps in preventing potential discrepancies between the intended asset value and the transferred ibts quantity.

Assessed type

ERC20

c4-bot-9 commented 8 months ago

Withdrawn by Bigsam