code423n4 commented 2 years ago

Handle

defsec

Vulnerability details

Impact

Zapper.sol uses OpenZeppelin’s safeApprove() which has been documented as 1) Deprecated because of approve-like race condition and 2) To be used only for initial setting of allowance (current allowance == 0) or resetting to 0 because it reverts otherwise.

The usage here is intended to allow increase of allowance when it falls low similar to the documented usage in Zapper.sol. Using it for that scenario will not work as expected because it will always revert if current allowance is != 0. The initial allowance is already set as uint256.max in constructor. And once it gets reduced, it can never be increased using this function unless it is invoked when allowance is reduced completely to 0.

Proof of Concept

The code can be observed from the following location.

https://github.com/code-423n4/2021-10-ambire/blob/main/contracts/wallet/Zapper.sol#L84

https://github.com/code-423n4/2021-10-ambire/blob/main/contracts/wallet/Zapper.sol#L92

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L52-L55

Tools Used

Recommended Mitigation Steps

Use safeIncreaseAllowance to increase the allowance to the maximum instead.

Ivshti commented 2 years ago

We don't use openzeppelin at all, so this is not relevant. There is some truth to the approve race condition mitigation, but it's already covered in other issues

GalloDaSballo commented 2 years ago

It seems like the contract for SafeERC20 is not in the repo for the Code423n4 contest, will ask the organizers

Ivshti commented 2 years ago

@GalloDaSballo it's in the main repo: https://github.com/AmbireTech/adex-protocol-eth/blob/224e4eb313eea87af7e05f55131c48bd2b3fd367/contracts/libs/SafeERC20.sol