search

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

2 stars 1 forks source link

The current implementation of the VotingEscrow contract doesn't support fee on transfer tokens #229

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#L418

Vulnerability details

Impact

Some ERC20 tokens implemented so a fee is taken when transferring them, for example STA and PAXG. The current implementation of the VotingEscrow contract will mess up the accounting of the locked amounts if token will be a token like that, what will lead to a state where users won't be able to receive their funds.

This will happen because the value that is added to the locked amount is not the actual value received by the contract, but the value supplied by the user (the value which the fee is taken from).

Proof of Concept

The STA token burns 1% of the value provided to the transfer function, which means the recipient gets only 99% of the transferred asset. Let's assume that token is the address of the STA token.

  1. Bob wants to lock 100 STA tokens and calls createLock(100 * 10**18, unlockTime).
  2. The addition to the locked amount variable is done with 100 * 10**18, while the actual amount that was received by the contract is 99 * 10**18.
  3. When the lock expires Bob will try to withdraw his tokens, and the transfer function will be called with the accounted locked amount (which is 100 * 10**18). This might succeed due to other users locking too, so the transferred tokens will be taken from "their tokens", but in the end there will be users left without an option to withdraw their funds, because the balance of the contract will be less than the locked amount that the contract is trying to transfer.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

Calculate the amount to add to the locked amount by the difference between the balances before and after the transfer instead of using the supplied value.

lacoop6tu commented 2 years ago

In our case, the token will be BalancerV2 Pool Token , which has no fee on transfer, in case someone else would like to fork this contract and use it, that fix will be required

gititGoro commented 2 years ago

Given that the warden couldn't know the use of Balancer only tokens, the severity will still be upheld.

elnilz commented 2 years ago

@gititGoro so should we explicitly exclude all weird implementations of e.g. ERC20 in the future in the contest docs? I mean there are other wild examples of ERC20 implementations that someone could point out would cause problems with this contract. I am not trying to discount the work of any warden here but I think the correct response here as well as for #231 would be to improve docs stating which ERC20 implementations are safe to use in combination with VotingEscrow

gititGoro commented 2 years ago

@elnilz This topic is very important to get right and the more it's debated, the more it's clear that there is no one size fits all answer. When I sponsored a contest, I only figured out after the fact the sorts of things that would have reduced unhelpful warden submissions, through no fault of C4. My suggestion is that we curate an open source optional questionnaire for sponsors. The more detail the sponsor gives a priori, the more we can mark unhelpful issues as invalid. As an example, the tools I use to deploy my dapps do not allow me to accidentally omit addresses in constructor arguments. So wardens who warn me that I don't check for the zero address in my constructors are not helping me. The questionnaire would cover many common case submissions such as that.

In your case, I had to dance a bit of a fine line: on the one hand, the wardens are not wrong in the event that you're unaware of token design nuance and so they shouldn't be penalized. On the other hand, this is a DAO and the wardens should at least suspect that the choice of token is a central decision. In the end, since both parties have good points to make and there's no clear decider, I chose to side with the wardens since it doesn't incur any additional cost to you as the sponsor and since it would seem unfair to penalize the wardens for an honest report with no flaws at the correct severity level.