Golom contest - 0xpiglet's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L71

Vulnerability details

Impact

An attacker can delegate multiple times, for each delegation adding extra voting power. Meaning that he can delegate to himself 100 times to have 100x voting power. This allows the attacker to easily force any proposal, as he can have more votes than the Ve total supply.

Proof of Concept

https://gist.github.com/0xpiglet/d194ebead29d1e5bb27ebe1bf2d9e2fb

Tools Used

Recommended Mitigation Steps

When delegating, it should clear the old delegation before delegating the power to someone else.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L94

Vulnerability details

Impact

The impact is twofold:

• When the first checkpoint for any tokenId is to be written, it will underflow when trying to fetch oldCheckpoint as nCheckpoints - 1 == -1. This means that the delegation flow is completely broken, and cannot be used.
• When a checkpoint have already been written in the same block, the in-memory value oldCheckpoint is updated, but the storage is never rewritten, making the call have no effect.

Proof of Concept

https://gist.github.com/0xpiglet/2badc8fb9dd0b610416852f8c1f81ee2

Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)

Tools Used

Recommended Mitigation Steps

To mitigate the underflow. Only fetch the old checkpoint if one exists, e.g., when nCheckpoints > 0. To update storage, replace the delegatedTokenIds inplace.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L71

Vulnerability details

Impact

An attacker can increase voting power at time t-1 (in the past). The delegate function pushes tokenId to the old checkpoint (in storage) before creating a new checkpoint. This means that the power of both checkpoints is altered, not only the new one. Can be used to increase voting power when voting on current proposals (not only future). See example below for increasing power in the past.

Proof of Concept

https://gist.github.com/0xpiglet/c88ce8e7d061e848469dd5753d868f33

Voting power tokenId 1 at 12: 999398909064507120
Voting power tokenId 1 at 11: 999398916991955115
tokenId 2 delegating to 1
Voting power tokenId 1 at 13: 1998797802274118250
Voting power tokenId 1 at 11: 1998797833983910230

Tools Used

Recommended Mitigation Steps

Pass the tokenId to _writeCheckpoint and only append it to the new/updated checkpoint.