[PDE-586] audit remediations 2 #150
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's new in this PR?
In YieldToken, it seems that in the current receiveYield implementation, when the YieldToken receives currency as yield, not only does the accrued yield increase, but the users' share values also inflated due to the conversion dependency on totalAssets held by YieldToken. This means users get additional value from the yield beyond what they should receive.
In AggregateToken, during buyVaultToken, it seems that the approval should be granted to the vault instead of the teller based on the BoringVault.enter source code.
Additionally, we recommend you not to trust user-provided address inputs. For example, during buyVaultToken, if the price off-chain calculation depends on the totalAsset held by the AggregateToken, users could pass a dummy teller address to inflate their share values. After redemption, due to the unused approval, users could still recover their asset.
Another thing to consider is whether it would make sense to limit this function to just MANAGER_ROLE, similar to buyComponentToken?
It seems that the current implementation of withdraw is using rounding down during calculating the shares. Users might be able to withdraw assets while burning fewer shares than they should. This can lead to loss of funds, especially when dealing with large share values.
It seems that in the NestStaking contract, users can deploy as many aggregate tokens as they want via createAggregateToken. If the list grows larger, the gas cost of the unfeatureToken function might increase significantly during iteration to find the correct item to remove.