FIAT DAO veFDT contest - 0xSky's results

Lines of code

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

Vulnerability details

Impact

VotingEscrow will not work for such tokens.

Proof of Concept

This issue is same as a previous contest. https://code4rena.com/reports/2022-05-factorydao/#m-03-safetransferfrom-is-recommended-instead-of-transfer-1

In the implementation of token transfer, the return value is checked after token transfer. But some tokens do not return bool values. After DM, I know you don't use these kinds of tokens, but this contract can be used on top of most of ERC20 tokens, and this can be a problem when we extend the usage.

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L507-L515

Vulnerability details

Checkpoint end time is not correct.

Impact

The user's voting power can be significantly decreased.

Proof of Concept

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L507-L515

At L513 of VotingEscrow.sol, when we increase unlock time, oldLocked.end should be oldUnlockTime instead of unlock_time. We can double-check it from the Checkpoint Math documentation here. https://github.com/code-423n4/2022-08-fiatdao/blob/main/CheckpointMath.md#increaseunlocktime

In the worst senario, if oldUnlockTime is 1 WEEK, and unlock_time is 1 YEAR, there will be some significant decrease of voting power in _checkpoint function. But I think this is very likely to a typo, so I will not add detailed showcase.

Tools Used

Manual Review

Recommended Mitigation Steps

-   oldLocked.end = unlock_time;
+   oldLocked.end = oldUnlockTime;