Spender can prevent operations to decrease its allowance and effectively spends all the granted allowance even though the owner was trying to remove the allowance.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L148-L159

Vulnerability details

Impact

• Owners who want to remove allowance from their spenders may not be able to remove the granted allowance, and instead, the spenders will be able to spend all the granted allowance even though the owner wanted to remove it.

Proof of Concept

• If an owner wants to remove all the allowance from a spender using the decreaseAllowance(), it needs to send the subtractedValue set to the exact total allowance that the spender has been granted.

  • The problem is that if the spender spends as little as 1 wei, the tx to decrease all the allowance will fail because the require() condition will not be true after the spender spends as little as 1 wei, the currentAllowance will be less than the subtractedValue

• Let's take as example: UserA (owner) grants 1000 allowance to UserB (spender), and after some time, the owner decides to remove all the allowance, so it sends a tx to remove 1000.

  • The spender has also sent a tx to spend 100 of the tokens from UserA (UserA is not aware of this tx), the tx of UserB is executed first, and then is executed the tx of the owner to remove all the granted allowance (1000) (but it fails, because at that point in time, the allowance was not anymore 1000, it was 900).
    • The spender realizes that the owner wants to remove all his allowance and proceeds to send another tx to spend all the remaining allowance.
    • The result is that the spender was able to spend the 1000 tokens, even though the owner tried to remove all the allowance from UserB (the spender).

function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool) {
    uint256 allowed = allowance[_msgSender()][spender];
    //@audit-info => If currentAllowance decreases from the time when the tx was sent until it was executed, and when the tx is been execute the currentAllowance is less than substractedValue, the tx will revert!
    require(allowed >= subtractedValue, "ERC20/insufficient-allowance");
    unchecked {
        allowed = allowed - subtractedValue;
    }
    allowance[_msgSender()][spender] = allowed;

    emit Approval(_msgSender(), spender, allowed);

    return true;
}

Tools Used

Manual Audit

Recommended Mitigation Steps

• Allow the spender to remove all the allowance without needing to explicitly specify the remaining amount of allowance, when removing all the allowance send the max value of an uint256 to indicate that all the remaining allowance must be decreased to 0.

• In this way, when owners want to reduce all the allowance, they won't need to explicitly specify the amount they want to reduce, instead, they just send the max value of uint256 and the code will automatically set the allowance of the specified spender to 0!

function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool) {
    //@audit-info => If substracTedValue is equal to the max uint256 value it means that the owner wants to decrease all the remaining allowance!
    if(substracTedValue == type(uint256).max) {
      _approve(msg.sender,spender,0)
      return true;
    } else {
      uint256 allowed = allowance[_msgSender()][spender];
      require(allowed >= subtractedValue, "ERC20/insufficient-allowance");
      unchecked {
          allowed = allowed - subtractedValue;
      }
      allowance[_msgSender()][spender] = allowed;

      emit Approval(_msgSender(), spender, allowed);

      return true;
    }
}

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #145

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid