search

code-423n4 / 2023-01-popcorn-findings

0 stars 0 forks source link

Assets that have a fee-on-transfer are not supported #737

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L153 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L193

Vulnerability details

Impact

Some tokens such as USDT have fee-on-transfer functionality that can be turned. With the current implementation a fee-on-transfer asset would issue too many shares since the fee is not accounted for.

Proof of Concept

When an asset is deposited in Vault in the deposit() function shares are calculated with 

shares = convertToShares(assets) - feeShares;

but the actual amount transferred will be less than asset since a fee is charged.

Tools Used

manual review

Recommended Mitigation Steps

Use the change in the asset.balanceOf(address(this)) of the vault to calculate shares.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid