Allowance isn't reduced on transfer if it is type(uint).max

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenModified.sol#L117 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenModified.sol#L133-L135

Vulnerability details

Impact

Allowance isn't reduced on transfer if it is type(uint).max. By design of the ERC20 token, if the spender is not the sender, allowance must always be deducted after the transfer.

Proof of Concept

    /* Get the allowance, infinite for the account owner */
    uint startingAllowance = 0;
    if (spender == src) {
      startingAllowance = type(uint).max;
    } else {
      startingAllowance = transferAllowances[src][spender];
    }

    uint allowanceNew = startingAllowance - tokens;

...

    /* Eat some of the allowance (if necessary) */
    if (startingAllowance != type(uint).max) {
      transferAllowances[src][spender] = allowanceNew;
    }

Case spender != src and transferAllowances[src][spender] == type(uint).max

Once token is transferred, transferAllowances[src][spender] won't get deducted as startingAllowance == type(uint).max

But in fact, if the spender is not src, transferAllowances[src][spender] must always be deducted after the transfer

Recommended Mitigation Steps

Using spender != src check instead

    /* Eat some of the allowance (if necessary) */
    if (spender != src) {
      transferAllowances[src][spender] = allowanceNew;
    }

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[tom2o17]

Compound fork code Out of scope.

Approving max is commonly used as a blankCheck method to allow a contract to send unlimited tokens (greater # than current US debt). I see no benefit to decrementing allowance on send if approval == uint256.max, as this value will reasonably never run out.

cc @ypatil12

[c4-sponsor]

tom2o17 marked the issue as sponsor disputed