code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Vault will drain if `_yieldVault` is fee-on-transfer token #357

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

On every withdraw/redeem, assets in the vault will decrease faster than expected and finally, the vault will drain.

Proof of Concept

Vault._withdraw assumes _yieldVault.withdraw(_assets, address(this), address(this)); will return the same amount of tokens as _assets but this is not the case if _yieldVault is a fee-on-transfer token. https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026-L1027

Tools Used

Manual review

Recommended Mitigation Steps

Calculate the difference of the asset and transfer the amount to the receiver.

+ uint256 assetBefore = IERC20(asset()).balanceOf(address(this)); 
_yieldVault.withdraw(_assets, address(this), address(this));
+ uint256 assetAfter = IERC20(asset()).balanceOf(address(this));
SafeERC20.safeTransfer(IERC20(asset()), _receiver, assetAfter - assetBefore);

Assessed type

Token-Transfer

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #470

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Out of scope

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory