Reentrancy can increase allowance can be used to take more funds than expected

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/contracts/enforcer/HolographERC20.sol#L420-L437
https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/contracts/enforcer/HolographERC20.sol#L363-L378

Vulnerability details

Reentrancy can increase allowance can be used to take more funds than expected

In the same way as the typical front run of ERC20 approve/transferFrom, you would be able to take money when calling maliciousERC20.beforeApprove() hook, this would realize a transferFrom using old allowance then reset value of allowance to the expected value and being able to still take the money out.

Proof of Concept

  function increaseAllowance(address spender, uint256 addedValue) public returns (bool) {
    uint256 currentAllowance = _allowances[msg.sender][spender];
    uint256 newAllowance;
    unchecked {
      newAllowance = currentAllowance + addedValue;
    }
    unchecked {
      require(newAllowance >= currentAllowance, "ERC20: increased above max value");
    }
    if (_isEventRegistered(HolographERC20Event.beforeApprove)) {
      require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));
    }
    _approve(msg.sender, spender, newAllowance);
    if (_isEventRegistered(HolographERC20Event.afterApprove)) {
      require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));
    }
    return true;
  }

External calls:
- [require(bool)(SourceERC20().beforeApprove(msg.sender,spender,newAllowance))](contracts/enforcer/HolographERC20.sol#L430)
State variables written after the call(s):
- [_approve(msg.sender,spender,newAllowance)](contracts/enforcer/HolographERC20.sol#L432)
	- [_allowances[account][spender] = amount](contracts/enforcer/HolographERC20.sol#L622)

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/contracts/enforcer/HolographERC20.sol#L420-L437

Also decrease and some ERC721 has reentrancy, however in withdrawing erc721 is not too effective

 function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) {
    uint256 currentAllowance = _allowances[msg.sender][spender];
    require(currentAllowance >= subtractedValue, "ERC20: decreased below zero");
    uint256 newAllowance;
    unchecked {
      newAllowance = currentAllowance - subtractedValue;
    }
    if (_isEventRegistered(HolographERC20Event.beforeApprove)) {
      require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));
    }
    _approve(msg.sender, spender, newAllowance);
    if (_isEventRegistered(HolographERC20Event.afterApprove)) {
      require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));
    }
    return true;
  }

External calls:
- [require(bool)(SourceERC20().beforeApprove(msg.sender,spender,newAllowance))](contracts/enforcer/HolographERC20.sol#L371)
State variables written after the call(s):
- [_approve(msg.sender,spender,newAllowance)](contracts/enforcer/HolographERC20.sol#L373)
	- [_allowances[account][spender] = amount](contracts/enforcer/HolographERC20.sol#L622)

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/contracts/enforcer/HolographERC20.sol#L363-L378

tools

Slither, manual analysis

Recommended Mitigation Steps

• Follow the CEI pattern.
• Consider using nonReentrant modifier to prevent reentrancy attack:
  OpenZeppelin/ReentrancyGuard.sol

[gzeoneth]

Looks valid but limited impact since it require

1. attacker control the source erc20 (which mean they can mint anyway); or
2. the source erc20.beforeApprove() hook pass execute flow to the attacker

[alexanderattar]

Source contract can already mint tokens so if the source contract gives access to attacker, it is outside of scope