The Contract Should Approve(0) first

[code423n4]

Handle

defsec

Vulnerability details

Impact

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-01-notional/blob/main/contracts/TreasuryManager.sol#L79

2. When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

[jeffywu]

We do not support USDT so we are not affected by this issue.

[pauliax]

Even if such tokens were supported, I do not think this is an issue, because the approve function is external and callable by the owner, so an owner can explicitly make 2 calls if necessary specifying a 0 amount in the first one.

    function approveToken(address token, uint256 amount) external onlyOwner {
        IERC20(token).approve(ASSET_PROXY, amount);
    }