search

code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

Unsafe usage of ERC20 transfer and transferFrom #231

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L425-L428 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L485-L488 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L546 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L657 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676

Vulnerability details

Impact

Some ERC20 tokens functions don't return a boolean, for example USDT, BNB, OMG. So the VotingEscrow contract simply won't work with tokens like that as the token.

Proof of Concept

The USDT's transfer and transferFrom functions doesn't return a bool, so the call to these functions will revert although the user has enough balance and the VotingEscrow contract won't work, assuming that token is USDT.

Tools Used

Manual auditing - VS Code, some hardhat tests and me :)

Recommended Mitigation Steps

Use the OpenZepplin's safeTransfer and safeTransferFrom functions

lacoop6tu commented 2 years ago

In our case the token is a BalancerV2 Pool Token which returns the boolean

gititGoro commented 2 years ago

This should be acknowledged, not disputed, since there is nothing in documentation suggesting the token is inherently safe to use.

elnilz commented 2 years ago

@gititGoro it's a no-issue in our specific case bc we will use VotingEscrow in combination with token which returns bool upon transfer/transferFrom. So at best this is a QA issue bc we should document that. some wardens actually asked us about what token we will be using pointing out the issue.

Now even if you'd want to award wardens who reported the issue it should then be a Med Risk bc if VotingEscrow is deployed with an unsafe token ppl would simply not be able to deposit into the contract but no funds would be at risk.

elnilz commented 2 years ago

fyi, even though we dont think this is an issue we will make use of safeTransfer and safeTransferFrom so its a helpful submission nonetheless

gititGoro commented 2 years ago

It's tokens like BNB that led me to maintain the high risk rating. For BNB, transferFrom returns a bool but transfer doesn't. In other words, users can stake but not unstake on any protocol that doesn't use safeTransfer.

I agree that wardens should contact sponsors but it's not a channel we can really monitor. So although the net result is a documentation fix rather than a bug fix, it's a documentation fix informed by the identification of a potentially show stopping bug rather than something like "Comment typo: it should be Bitcoin, not bit coin".