OpenZeppelin's safeApprove() will revert if the account already is approved and the new safeApprove() is done with a non-zero value
function safeApprove(
IERC20 token,
address spender,
uint256 value
) internal {
// safeApprove should only be called when setting an initial allowance,
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require(
(value == 0) || (token.allowance(address(this), spender) == 0),
"SafeERC20: approve from non-zero to non-zero allowance"
);
_callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}
Customers cannot be topped up a second time, which will cause them to be liquidated even though they think they're protected
Proof of Concept
There are multiple places where safeApprove() is called a second time without setting the value to zero first. The instances below are all related to topping up.
Compound-specific top-ups will fail the second time around when approving the ctoken again:
Lines of code
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L71 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L120 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/AaveHandler.sol#L53 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L847
Vulnerability details
OpenZeppelin's
safeApprove()
will revert if the account already is approved and the newsafeApprove()
is done with a non-zero valuehttps://github.com/OpenZeppelin/openzeppelin-contracts/blob/fcf35e5722847f5eadaaee052968a8a54d03622a/contracts/token/ERC20/utils/SafeERC20.sol#L45-L58
Impact
Customers cannot be topped up a second time, which will cause them to be liquidated even though they think they're protected
Proof of Concept
There are multiple places where
safeApprove()
is called a second time without setting the value to zero first. The instances below are all related to topping up.Compound-specific top-ups will fail the second time around when approving the
ctoken
again:https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L50-L71
Compound-specific top-ups will also fail when trying to repay debt:
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/CompoundHandler.sol#L62-L65
Aave-specific top-ups will fail for the
lendingPool
:https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/handlers/AaveHandler.sol#L36-L53
The
TopUpAction
itself fails for thefeeHandler
:https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L840-L847
I've filed the other less-severe instances as a separate medium-severity issue, and flagged the remaining low-severity instances in my QA report
Tools Used
Code inspection
Recommended Mitigation Steps
Always do
safeApprove(0)
if the allowance is being changed, or usesafeIncreaseAllowance()