search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

Not safe `transferFrom` #157

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Safe.sol#L9 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L128

Vulnerability details

Impact

The Safe library says:

@dev Caution! This library won't check that a token has code, responsibility is delegated to the caller.

But this check is not made in Swivel contract, so the Safe library it's prone to phantom methods attacks. Supposedly it is a library to make transfers more secure, but it's actually insecure.

A library that pretends to work better with tokens that do not comply with the ERC20 standard, by not returning a boolean, which avoids possible attacks that have happened in the past in other projects, cannot be considered safe.

Proof of Concept

If you try to call the Safe methods with the following token 0x000000000000000000000000000000000000000000000 (or any EOA) you can see that the transaction is successful, you have to check that the address is a valid contract and maybe check that the call returns a certain data. Otherwise it is possible to call it and lose tokens or produce undesired errors.

This remind me to this attack https://certik.medium.com/qubit-bridge-collapse-exploited-to-the-tune-of-80-million-a7ab9068e1a0 where we can see:

One of the root causes of the vulnerability was the fact that tokenAddress.safeTransferFrom() does not revert when the tokenAddress is the zero (null) address (0x0…000).

Recommended Mitigation Steps

scaraven commented 2 years ago

I do not understand how user funds are at risk. Each specific marketplace is specific to the underlying token, maturity date and protocol. If a user wants to deposit funds into a market where the underlying token is not a valid contract, they will only deal with that specific address. A user can't lose funds from an ERC20 contract which does not exist.

JTraversa commented 2 years ago

Generally agree with scaravan. Ofc we control the launch of markets meaning there isnt really a risk to begin with but beyond that, all of scaravan's points are pretty valid.

bghughes commented 2 years ago

I disagree with this statement: "A user can't lose funds from an ERC20 contract which does not exist" (zero address exploits are legitimate and can cause unexpected behavior), but agree with @scaraven and @JTraversa here.

In my analysis, the zero address call to transferFrom seems impossible as it passes in address(this) in the call that the warden identified.

That said, the warden is correct that the library notes the unsafe usage of the zero address. Generally, this is informational, out-of-scope, and irrelevant IMO especially given:

So the only things explicitly NOT in scope:

Details of 5095 implementation (the EIP isnt final yet) A few external Libs:

  • FixedPointMathLib
  • LibCompound
  • LibFuse
  • Safe.sol Some older stuff not worth a review:
  • Hash.sol
  • Sig.sol