Pool.sol & Synth.sol: Failing Max Value Allowance

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

In the _approve function, if the allowance passed in is type(uint256).max, nothing happens (ie. allowance will still remain at previous value). Contract integrations (DEXes for example) tend to hardcode this value to set maximum allowance initially, but this will result in zero allowance given instead.

This also makes the comment // No need to re-approve if already max misleading, because the max allowance attainable is type(uint256).max - 1, and re-approval does happen in this case.

This affects the approveAndCall implementation since it uses type(uint256).max as the allowance amount, but the resulting allowance set is zero.

Recommended Mitigation Steps

Keep it simple, remove the condition.

function _approve(address owner, address spender, uint256 amount) internal virtual {
        require(owner != address(0), "!owner");
        require(spender != address(0), "!spender");
        _allowances[owner][spender] = amount;
        emit Approval(owner, spender, amount);
    }

[SamusElderg]

We acknowledge the issue in the max approval for approveAndCall, which we don't use. Furthermore, the issue is only a problem if a user directly approves a maximum possible amount which would mean they are assuming trust in the contract.

We will also change _approve in the pool and synth contracts. Risk, as outlined above, is low.

[ghoul-sol]

This is high risk as explained in #152