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

ERC20 approve method missing return value check #36

Open
code423n4 opened this issue Oct 24, 2021 · 2 comments
Open

ERC20 approve method missing return value check #36

code423n4 opened this issue Oct 24, 2021 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working

Comments

@code423n4
Copy link
Contributor

Handle

defsec

Vulnerability details

Impact

The lock, _buyMochi, claimRewardAsMochi, _buyCRV, _lockCRV functions perform an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

  1. Navigate to "https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L109"
  2. The lock, _buyMochi, claimRewardAsMochi, _buyCRV, _lockCRV functions use an approve method.
  3. Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Code Locations

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/treasury/MochiTreasuryV0.sol#L86

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/treasury/MochiTreasuryV0.sol#L97

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/feePool/FeePoolV0.sol#L69

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/feePool/ReferralFeePoolV0.sol#L34

https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/emission/VestedRewardPool.sol#L55

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.1/contracts/token/ERC20/utils/SafeERC20.sol#L74

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Oct 24, 2021
code423n4 added a commit that referenced this issue Oct 24, 2021
@r2moon r2moon added the invalid This doesn't seem right label Oct 27, 2021
@r2moon
Copy link
Collaborator

r2moon commented Oct 27, 2021

If approve failed, then further tx will fail.

@ghoul-sol
Copy link
Collaborator

per sponsor comment, best practice

@ghoul-sol ghoul-sol added 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments invalid This doesn't seem right labels Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants