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

12 stars 7 forks source link

Some tokens cannot be used as underlying assets #363

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#L282

Vulnerability details

Impact

This incompatibility restricts the protocol's application to certain tokens, specifically those that do not support the approval of type(uint256).max.

Proof of Concept

In the Vault.sol contract, the constructor's assets's approval defaults to type(uint256).max as seen at L282.

    // Approve once for max amount
    asset_.safeApprove(address(yieldVault_), type(uint256).max);

However, not all tokens support this type of approval.

Tool Used

Manual Audit

Recommended Mitigation Steps

An explicit statement should be made to not support such tokens as underlying assets, or if needed a research could be made to find out the max posssible approval for a token,

Assessed type

Context

Picodes commented 1 year ago

To support your case for Medium severity please provide at least one credible example of such token

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

PierrickGT commented 1 year ago

I agree, difficult to know which tokens to test to see if our implementation would break. We use the (brokentoken)[https://github.com/GenerationSoftware/brokentoken] library to test ERC-20 tokens that behaves unexpectedly, so this kind approval error should be covered.