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

0 stars 0 forks source link

No ERC20 `safeApprove` versions called #20

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Calls to .approve with user-defined tokens are made in:

PoolCommitter.setQuoteAndPool

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens.

kumar-ish commented 3 years ago

The warden's assessment isn't correct because pool committer doesn't hold any funds -- the approve call is an oversight and will be removed

GalloDaSballo commented 3 years ago

This one is nuanced:

For scoring the competition, the finding is valid, the warden has identified a potential way to make the function revert when using ERC20s that don't return a boolean for their approve function

As such the finding is valid, severity is correct as well as this can make the protocol behave in unexpected ways.

That said, the sponsor affirms that the contract isn't even supposed to have the approve function in there, if that's the case, and the sponsor removes the extra code, there is no vulnerability in the code.

So for scoring purposes I'll keep it as Low Risk and valid

However for mitigation purposes, deleting the approve is sufficient

kumar-ish commented 3 years ago

@GalloDaSballo Oops, I forgot to link it here; the contract isn't supposed to have an approve call and so, quote tokens that don't return booleans aren't going to affect markets in any case. The removal of the approve call is here: https://github.com/tracer-protocol/perpetual-pools-contracts/pull/180/commits/3dcbee541cf11f2632ecf1cb3ff722f6c64c5ec8

kumar-ish commented 3 years ago

this can make the protocol behave in unexpected ways

Could you elaborate on this?

GalloDaSballo commented 3 years ago

@kumar-ish Calling safeApprove can revert if you're changing the allowance from non-zero to non-zero, classic example is USDT

I believe OpenZeppelin recommend using safeIncreaseAllowance as most developers use safeApprove in the way