Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_payoutToken[s]() is not compatible with ERC20-tokens which revert on zero value transfer #454

Closed
Tracked by #93
code423n4 opened this issue Oct 25, 2022 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate This issue or pull request already exists flag for judge Judge should review this issue responded The Holograph team has reviewed and responded sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/enforcer/PA1D.sol#L317
https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/enforcer/PA1D.sol#L340

Vulnerability details

Impact

Payout is blocked.

Proof of Concept

PA1D._payoutToken() and PA1D._payoutTokens() call ERC20.transfer() to send tokens to a list of payout recipients.
Some tokens (e.g. LEND) revert when transferring a zero value amount. If one of the recipients is to receive a zero amount of such a token, then the entire transaction will revert and the payout cannot be made.

Tools Used

Code inspection

Recommended Mitigation Steps

Only attempt to transfer to a recipient if the amount (sending) is greater than zero.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 25, 2022
code423n4 added a commit that referenced this issue Oct 25, 2022
@gzeoneth gzeoneth added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Oct 28, 2022
@gzeoneth
Copy link
Member

can be checked offchain, risk is relatively low

@alexanderattar
Copy link

Low severity. We can add a check just in case

@alexanderattar alexanderattar added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") responded The Holograph team has reviewed and responded labels Nov 8, 2022
@ACC01ADE
Copy link

Duplicate of #456

@ACC01ADE ACC01ADE marked this as a duplicate of #456 Nov 17, 2022
@ACC01ADE ACC01ADE added the flag for judge Judge should review this issue label Nov 17, 2022
@gzeoneth
Copy link
Member

Duplicate of #456

While not exactly a duplicate, I agree to consolidate all issue regarding ERC20 incompatibility into 1 Med risk issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate This issue or pull request already exists flag for judge Judge should review this issue responded The Holograph team has reviewed and responded sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants