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

Avoid the use of increaseAllowance and decreaseAllowance from ERC20 that are recently deprecated #21

Closed
c4-submissions opened this issue Oct 29, 2023 · 5 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 insufficient quality report This report is not of sufficient quality sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/EBTCToken.sol?plain=1#L152-L170

Vulnerability details

Impact

Recently, the increaseAllowance function has been removed from the OpenZeppelin ERC20 contract due to its exploitation in phishing attacks and to prevent the possibility of further phishing attacks.

See (OpenZeppelin/openzeppelin-contracts#4583](OpenZeppelin/openzeppelin-contracts#4583) . We should remove the functions increaseAllowance and decreaseAllowance as they only solve an imaginary problem.

These functions are not part of the EIP-20 specs.

Using these functions will result in unexpected behaviour which will cause contracts to become corrupted.

These functions may allow for further phishing possibilities.

Proof of Concept

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/EBTCToken.sol?plain=1#L152-L170

    function increaseAllowance(
        address spender,
        uint256 addedValue
    ) external override returns (bool) {
        _approve(msg.sender, spender, _allowances[msg.sender][spender] + addedValue);
        return true;
    }

    function decreaseAllowance(
        address spender,
        uint256 subtractedValue
    ) external override returns (bool) {
        uint256 cachedAllowances = _allowances[msg.sender][spender];
        require(cachedAllowances >= subtractedValue, "ERC20: decreased allowance below zero");
        unchecked {
            _approve(msg.sender, spender, cachedAllowances - subtractedValue);
        }
        return true;
    }

Tools Used

Manual code review

Recommended Mitigation Steps

considering removing increaseAllowance/decreaseAllowance function from EBTCToken contract.

Or implement a function similar to this SafeERC20 library which is still available.

Assessed type

ERC20

@c4-submissions c4-submissions 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 29, 2023
c4-submissions added a commit that referenced this issue Oct 29, 2023
@bytes032
Copy link

  • Not a vulnerability
  • Over inflated severity

@c4-pre-sort
Copy link

bytes032 marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 15, 2023
@c4-sponsor
Copy link

GalloDaSballo (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 20, 2023
@GalloDaSballo
Copy link

We could call it dog.bark and it would still just be an opinion

@c4-judge
Copy link

jhsagd76 marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 25, 2023
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 insufficient quality report This report is not of sufficient quality sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants