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

fix: ensure reward fees are only transferred if the reward token is set #36

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

jparklev
Copy link
Contributor

@jparklev jparklev commented Sep 2, 2024

No description provided.

stevenvaleri and others added 2 commits September 2, 2024 11:20

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>
Copy link
Contributor

@stevenvaleri stevenvaleri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative to consider would be using a safeTransfer that has this check included. That would be a significant dependency shift, and given the audits already performed, I think this solution makes sense.

All instances of rewardToken.safeTransfer are protected by an is 0 check and there's a test-case for each expected error ✅

@jparklev
Copy link
Contributor Author

jparklev commented Sep 3, 2024

The only alternative to consider would be using a safeTransfer that has this check included. That would be a significant dependency shift, and given the audits already performed, I think this solution makes sense.

All instances of rewardToken.safeTransfer are protected by an is 0 check and there's a test-case for each expected error ✅

Looking at this again, i wonder if we should actually not revert on collect fees but instead just skip the reward transfer. That way we could still collect pToken fees. What do you think @stevenvaleri?

@stevenvaleri
Copy link
Contributor

Looking at this again, i wonder if we should actually not revert on collect fees but instead just skip the reward transfer. That way we could still collect pToken fees. What do you think @stevenvaleri?

that's a very good idea. in addition to avoiding the issue identified where the rewardToken is set from an address to zero, it enables collecting pTokens fees for points that just don't have a redemption token yet.

@jparklev
Copy link
Contributor Author

jparklev commented Sep 3, 2024

it enables collecting pTokens fees for points that just don't have a redemption token ye

I think this case would be safe regardless, since you can't accrue reward token fees until a reward token is added, so main concern is just when it's added, then removed still ~

…just skip the transfer
@jparklev
Copy link
Contributor Author

jparklev commented Sep 3, 2024

How does this look @stevenvaleri? 8dc5fee

@stevenvaleri
Copy link
Contributor

How does this look @stevenvaleri? 8dc5fee

looks good

@jparklev jparklev changed the base branch from main to fix-review September 4, 2024 00:45
@jparklev jparklev merged commit 6415873 into fix-review Sep 4, 2024
2 checks passed
jparklev added a commit that referenced this pull request Sep 7, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* fix: correct handling of non-18 decimal reward tokens (#35)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* fix: ensure reward fees are only transferred if the reward token is set, sherlock 185

* fix: correct handling of non-18 decimal reward tokens

* fix: revert rewards collection change from different branch

* fix: revert rewards collection test change from different branch

* fix: correct reversion commit

---------

Co-authored-by: Steven Valeri <[email protected]>

* fix: ensure reward fees are only transferred if the reward token is set (#36)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* fix: ensure reward fees are only transferred if the reward token is set

* refactor: don't revert on collect fees if the reward token is unset, just skip the transfer

---------

Co-authored-by: Steven Valeri <[email protected]>

* chore: trusted claimer -> trusted receiver (#37)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* chore: trusted claimer -> trusted receiver

---------

Co-authored-by: Steven Valeri <[email protected]>

* fix: separate deposit cap tracking from contract balance changes (#38)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* fix: separate deposit cap tracking from contract balance changes

* chore: move interaction below effects for deposit

---------

Co-authored-by: Steven Valeri <[email protected]>

* Chore/agpl (#40)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* chore: AGPL

---------

Co-authored-by: Steven Valeri <[email protected]>

* Chore/collect fees after effects (#39)

* Feat/mapping comment update (#18)

* feat: update mapping comments

* feat: clean comment

* Update contracts/PointTokenVault.sol

Co-authored-by: Josh Levine <[email protected]>

---------

Co-authored-by: Josh Levine <[email protected]>

* chore: collect fees after effects

---------

Co-authored-by: Steven Valeri <[email protected]>

* feat: add ExecutionFailed error and improve error handling in execute function (#42)

* chore: lock sol version to 0.8.24 (#41)

* chore: add comment about not supporting > 18 decimals (#43)

---------

Co-authored-by: Steven Valeri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants