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

Use of deprecated safeApprove() function #5

Closed
code423n4 opened this issue Apr 14, 2022 · 1 comment
Closed

Use of deprecated safeApprove() function #5

code423n4 opened this issue Apr 14, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L133
https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L136

Vulnerability details

Impact

In CitadelMinter.sol the initialize() function makes use of safeApprove(). OpenZeppelins safeApprove() implementation is deprecated. Using this deprecated function can lead to unintended reverts and potentially the locking of user funds.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L133

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L136

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38

OpenZeppelin/openzeppelin-contracts#2219

Tools Used

Manual code review

Recommended Mitigation Steps

Consider replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance()

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Apr 14, 2022
code423n4 added a commit that referenced this issue Apr 14, 2022
@shuklaayush shuklaayush added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels May 11, 2022
@shuklaayush
Copy link
Collaborator

I think using safeApprove is fine for infinite approvals done only once. It's only an issue when you're modifying approvals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants