code423n4 commented 3 years ago

Handle

defsec

Vulnerability details

Impact

exchangeV2() calls Uniswap’s swapExactTokensForTokens to swap tokens to tokens. This will work if msg.sender, i.e. WalletZapper contract, has already given the router an allowance of at least amount on the input token.

Given that there is no prior approval, the call to UniswapV2 router for swapping will fail because msg.sender has not approved UniswapV2 with an allowance for the tokens being attempted to swap.

The impact is that exchangeV2() will fail and revert while working with token. (for instance DAI)

Proof of Concept

Navigate to "https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L113" and "https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L115"

Tools Used

Manual Code Review

Recommended Mitigation Steps

Add token approval to router with an allowance for the tokens being attempted to swap.

Ivshti commented 3 years ago

intended behavior, the Zapper is supposed to pre-approve contracts it will interact with

GalloDaSballo commented 3 years ago

Approval to the uniswap router can be granted via approve it may or may not be helpful for the sponsor to add all approvals atomically in the same function. However the finding is invalid as there are ways to grant approval to the router

code-423n4 / 2021-10-ambire-findings