hats-finance / ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4

Smart Contracts for Ether Fi dapp
1 stars 1 forks source link

Unhandled arithmetic overflow in increaseAllowance in EETH.sol #22

Open hats-bug-reporter[bot] opened 11 months ago

hats-bug-reporter[bot] commented 11 months ago

Github username: -- Submission hash (on-chain): 0x7a2fae9da62e384b042714c294fd9299eb43180b62c0243b391dea2dc4cb133c Severity: low

Description: Description\ In order to keep consistency of how errors are handled it can be considered to slightly alter the code of increaseAllowance.

The method for decreaseAllowance handled arithmetic under flow with a require statement that checks that the resulting subtraction will not error as below

    function decreaseAllowance(address _spender, uint256 _decreaseAmount) external returns (bool) {
        address owner = msg.sender;
        uint256 currentAllowance = allowance(owner, _spender);
        require(currentAllowance >= _decreaseAmount, "ERC20: decreased allowance below zero");
        unchecked {
            _approve(owner, _spender, currentAllowance - _decreaseAmount);
        }
        return true;
    }

However the increase allowance would revert with an arithmetic overflow if the user tried to increase to allowance beyond type(uint256).max.

The currently code is below

    function increaseAllowance(address _spender, uint256 _increaseAmount) external returns (bool) {
        address owner = msg.sender;
        uint256 currentAllowance = allowance(owner, _spender);
        _approve(owner, _spender,currentAllowance + _increaseAmount);
        return true;
    }

Attack Scenario\ Although this could not be exploited for consistency it can be considered to add checks for arithmetic overflow

  1. Proof of Concept (PoC) File Please copy and paste to code below into the test directory file EETH.t.sol The code adds a check to ensure that the increaseAmount added to the currentAllowance variable.
    function increaseAllowance(address _spender, uint256 _increaseAmount) external returns (bool) {
        address owner = msg.sender;
        uint256 currentAllowance = allowance(owner, _spender);
        uint256 allowedIncrease = type(uint256).max - currentAllowance;
        require(_increaseAmount <= allowedIncrease, "ERC20: increase allowance exceeds max allowance");
        _approve(owner, _spender,currentAllowance + _increaseAmount);
        return true;
    }
  1. Revised Code File (Optional) It can be considered to change the code as below
    
    diff --git a/src/EETH.sol b/src/EETH.sol
    index a58e44ea..607dfb98 100644
    --- a/src/EETH.sol
    +++ b/src/EETH.sol
    @@ -86,6 +86,8 @@ contract EETH is IERC20Upgradeable, UUPSUpgradeable, OwnableUpgradeable, IERC20P
     function increaseAllowance(address _spender, uint256 _increaseAmount) external returns (bool) {
         address owner = msg.sender;
         uint256 currentAllowance = allowance(owner, _spender);
    +        uint256 allowedIncrease = type(uint256).max - currentAllowance;
    +        require(_increaseAmount <= allowedIncrease, "ERC20: increase allowance exceeds max allowance");
         _approve(owner, _spender,currentAllowance + _increaseAmount);
         return true;
     }
seongyun-ko commented 11 months ago

if it happens, the txn will revert anyway. does not expose any attack vector