code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Any ERC4626 vault has an underlying ERC20 representing the assets in the vault. setLiquidationPair is an owner-controlled function that set a max approve to an owner-controlled address. This hands over control of the underlying assets to this external address, which can allow the owner to set their own malicious address and use transferFrom to withdraw all assets from the Vault.

Proof of Concept

The max approve happens on this line

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

The liquidationPair_ address is set by the owner and there are no checks on this address besides confirming it is not the zero address. Once approved, a malicious owner can use their own malicious contract to withdraw all assets.

Tools Used

Manual review

Recommended Mitigation Steps

Add some safeguards for depositors so that the Vault owner does not have complete control over the vault assets. This is not a simple fix with the existing value design. The new version of PoolTogether is intended to have greater decentralization, so limiting the owner's abilities to control crucial values is important.

Assessed type

Rug-Pull

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #324