IF THE UNDERLYING ASSET IS A FEE ON TRANSFER TOKEN IT COULD BREAK THE INTERNAL ACCOUNTING OF THE VAULT

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L951-L956
https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L959
https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026-L1027

Vulnerability details

Impact

The Vault._deposit function is used by the users to deposit _assets to the vault and mint vault shares to the recipient address. The amount of _assets are transferred to the Vault as follows:

  SafeERC20.safeTransferFrom(
    _asset,
    _caller,
    address(this),
    _assetsDeposit != 0 ? _assetsDeposit : _assets
  );

The Vault.deposit function uses this _assets amount to calculate the number of shares to be minted to the _recipient address.

The issue here is if the underlying _asset is a fee on transfer token then the actual received amount to the vault will be less than what is referred in the Vault.deposit function _assets input parameter. But the shares to mint is calculated using the entire _assets amount.

This issue could be further aggravated since the _asset is again deposited to the _yieldVault and when needing to be redeemed will be withdrawn from the _yieldVault as well. These operations will again charge a fee if the _asset is a fee on transfer token. Hence the actual left _asset amount for particular user will be less than the amount he initially transferred in.

Hence when the user redeems the minted shares back to the _assets, the contract will not have enough assets to transfer to the redeemer thus reverting the transaction.

Proof of Concept

      SafeERC20.safeTransferFrom(
        _asset,
        _caller,
        address(this),
        _assetsDeposit != 0 ? _assetsDeposit : _assets
      );

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L951-L956

    _yieldVault.deposit(_assets, address(this));

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L959

    _yieldVault.withdraw(_assets, address(this), address(this));
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026-L1027

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to compute the _assets amount balance of the contract before and after the safeTransferFrom call and get the difference between the two as the actually transferred amount to the Vault. Then this actually transferred amount can be converted to shares and mint the correct amount of shares to the recipient.

  uint256 balanceBefore = _asset.balanceOf(address(this));

      SafeERC20.safeTransferFrom(
        _asset,
        _caller,
        address(this),
        _assetsDeposit != 0 ? _assetsDeposit : _assets
      );

  uint256 balanceAfter = _asset.balanceOf(address(this));  

  uint256 transferredAmount = balanceAfter - balanceBefore;

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[Picodes]

Out of scope per the automated report: https://gist.github.com/itsmetechjay/e7fd03943bbacff1984a33b9f89c4149 (MEDIUM-4)

[c4-judge]

Picodes marked the issue as unsatisfactory:
Out of scope

[udsene]

@Picodes, The fee on transfer token vulnerability described in this issue (#470) is related to the token transfers happening in the Vault.sol contract. How the fee charged on token transfers could effect the Vault._deposit , _yieldVault.deposit and _yieldVault.withdraw functions.

The MEDIUM-4 given in the automated report has found one instance of this vulnerability (fee on token transfer) and it is related to the PrizePool.sol contract and not the Vault.sol contract. It is not the same vulnerability present in the #470 issue.

[Picodes]

Indeed. The automated report didn't properly flag that Vault.sol won't work with FoT.

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as satisfactory