Golom contest - panprog's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298-L305

Vulnerability details

Impact

RewardDistributor smart contract's VoteEscrow (both ve and pendingVoteEscrow) is not initialized on creation (equals to address(0)). When trying to set ve by calling RewardDistributor.addVoteEscrow(_voteEscrow), ve is set to pendingVoteEscrow, which equals to address(0). This makes it impossible to set VoteEscrow in RewardDistributor, as any call to addVoteEscrow just always sets ve to address(0). pendingVoteEscrow is also never assigned due to this bug, as ve is always set to address(0).

Due to this bug, all trading fees received by RewardDistributor are stuck since nobody can claim them, because claiming the fees requires ve to be not address(0), but it's always address(0):

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L173

Proof of Concept

Copy this file to test/ and run:

npx hardhat test test/VoteEscrowDelegation.specs.ts

See test result: #RewardDistributor.addVoteEscrow

https://gist.github.com/panprog/2f1b482d89093d25e40a8b352dadd69c

Recommended Mitigation Steps

Set ve to _voteEscrow instead of pendingVoteEscrow if ve == address(0) in addVoteEscrow:

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
            ve = VE(_voteEscrow);
        } else {
            voteEscrowEnableDate = block.timestamp + 1 days;
            pendingVoteEscrow = _voteEscrow;
        }
    }

Lines of code

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

Vulnerability details

Impact

In VoteEscrowDelegation.delegate function, nCheckpoints variable equals 0 for all tokens initially. When it's 0, _writeCheckpoint is called with nCheckpoints == 0. The first line (after require) in _writeCheckpoint has nCheckpoints - 1 expression which reverts with overflow when nCheckpoints == 0 (solidity version used is 0.8.11, which automatically checks unsigned subtraction overflows). Since nCheckpoints can only increase via calling delegate, this means that delegate always reverts.

Since delegate always reverts, VoteEscrowDelegation is useless and voting is impossible, potentially causing severe damage to project which can't execute governance voting (as getVotes for all tokens returns 0 without delegations).

Proof of Concept

Copy this file to test/ and run:

npx hardhat test test/VoteEscrowDelegation.specs.ts

See test result: #delegate - delegate should succeed

https://gist.github.com/panprog/2f1b482d89093d25e40a8b352dadd69c

Recommended Mitigation Steps

Check for edge case of nCheckpoints == 0:

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

        if (nCheckpoints == 0) {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
            return;
        }

        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

        if (oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

Lines of code

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

Vulnerability details

Impact

When delegate function is called, the delegated tokenId is never checked to see if it was already delegated, making it possible to delegate the same tokenId as many times as you want. It can be delegated to itself or to any other tokenIds.

Being able to delegate the same tokenId many times leads to multiple possible exploits, for example:

1. The most severe - ability to inflate voting power by multiplying one token's voting power by any multiple. Just delegate tokenId to itself as many times as you can, each delegate increases its token voting power. This allows any malicious user to win any voting.

2. removeDelegation removes tokenId instance from delegation array only once. So if it's delegated multiple times, removeDelegation only removes it once, still keeping delegation of the other delegations.

3. _transferFrom function calls removeDelegation internally, so when token is transferred, not all delegations might be cleared.

Proof of Concept

Note: currently delegate call always fails due to the other bug reported. In order to run these tests, this bug must be fixed first by fixing _writeCheckpoint to:

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

        if (nCheckpoints == 0) {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
            return;
        }

        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

        if (oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

After that fix, copy the file from the link to test/ and run:

npx hardhat test test/VoteEscrowDelegation.specs.ts

See test result: #delegate - delegate the same token twice

https://gist.github.com/panprog/2f1b482d89093d25e40a8b352dadd69c

Recommended Mitigation Steps

It seems that delegates array is supposed to keep track of delegates, but it is only assigned once and never checked or cleared anywhere else. This has to be fixed in a couple of places.

In delegate function:

        require(delegates[tokenId] == 0, 'VEDelegation: tokenId already delegated');
        delegates[tokenId] = toTokenId;

In removeDelegation function:

        removeElement(checkpoint.delegatedTokenIds, tokenId);
	delegates[tokenId] = 0;

Lines of code

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

Vulnerability details

Impact

When removeDelegation function is called, the tokenId is removed from tokenId's delegation array only. This only removes tokenId from delegations to itself, but doesn't remove it from delegations to any other tokens. Moreover, removeDelegation reverts if tokenId was not delegated to, and since _transferFrom calls removeDelegation, _transferFrom also reverts if token was not delegated to.

Inability to removeDelegation (including removal before transfering to the other owner) seriously impacts any voting, because voters can't re-delegate their votes or remove delegation. Moreover, tokens without delegations to them can not be transferred as this operation reverts, essentially making users lose some control over their staked tokens (can't transfer them, can't change delegations).

Proof of Concept

Note: currently delegate call always fails due to the other bug reported. In order to run these tests, this bug must be fixed first by fixing _writeCheckpoint to:

    function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

        if (nCheckpoints == 0) {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
            return;
        }

        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

        if (oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        } else {
            checkpoints[toTokenId][nCheckpoints] = Checkpoint(block.number, _delegatedTokenIds);
            numCheckpoints[toTokenId] = nCheckpoints + 1;
        }
    }

After that fix, copy the file from the link to test/ and run:

npx hardhat test test/VoteEscrowDelegation.specs.ts

See test result: #delegate - remove delegation.

https://gist.github.com/panprog/2f1b482d89093d25e40a8b352dadd69c

Recommended Mitigation Steps

Make use of delegates array to remove delegation from delegates[tokenId] rather than from tokenId itself and don't forget to clear delegates array for the token.

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        if (delegates[tokenId] != 0) {
            uint256 nCheckpoints = numCheckpoints[delegates[tokenId]];
            Checkpoint storage checkpoint = checkpoints[delegates[tokenId]][nCheckpoints - 1];
            removeElement(checkpoint.delegatedTokenIds, tokenId);
            _writeCheckpoint(delegates[tokenId], nCheckpoints, checkpoint.delegatedTokenIds);
            delegates[tokenId] = 0;
        }
    }