search

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

0 stars 0 forks source link

Deposits don't work with fee-on transfer tokens #17

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom(). Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).

Impact

The PoolCommiter.commit() function will store the entire amount in the commitment but with fee-on-transfer tokens, fewer tokens will be transferred which leads to inconsistencies with the pool.longBalance() and in uncommit.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring routines

kumar-ish commented 3 years ago

Only governance (a multisig) can deploy markets, and has complete say over what markets can be deployed (see the onlyGov modifier in PoolFactory.sol#deployPool). Because new markets being deployed would be done via proposal to the DAO, which include the collateral token being used in a proposed market, markets with fee-on transfer tokens like Aave's aTokens just won't be deployed. I think this is a fairly safe assumption to make and thus we're making it out of scope. In any case, the chances of this happening and slipping past everyone who votes in the proposals and not being noticed extremely soon after a market is deployed are extremely low.

GalloDaSballo commented 3 years ago

I think this is a valid finding, the warden has shown a way to tamper with the protocol, extracting value (as such medium severity)

In terms of mitigation, not using feeOnTransfer or rebasing tokens is completely legitimate