Golom contest - Green's results

Lines of code

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

Vulnerability details

Impact

It is possible to manipulate the voting power of a given NFT because a particular tokenId can repeatedly be added to delegatedTokenIds for the same toToken.

Additionally, a particular tokenId can be listed as delegated for multiple toTokens. Therefore, inflating voting power without bound.

Proof of Concept

If I own NFT X, and I delegate to NFT Y 100 times, checkpoints[Y][nCheckpoints - 1].delegatedTokenIds would contain an array with X 100 times. As a result, when getVotes(Y) is called, the balance of X is included 100 times so the vote count is exacerbated.

Another scenario is if I own NFT X, and I delegate to NFT Y and then I delegated to NFT Z. There will now be 2 checkpoints, checkpoints[Y][nCheckpoints - 1] and checkpoints[Z][nCheckpoints - 1], with the delegatedTokenIds as [X] for both. When getVotes(Y) is called ("gets the current votes balance"), the balance of X is included although delegates[tokenId] == Z.

Tools Used

Remix

Recommended Mitigation Steps

In the delegate function, if there is a previous entry in the delegates[tokenId] mapping that was made, the tokenId should be removed from the ​​checkpoint.delegatedTokenIds to avoid duplicating the voting power of a single NFT.

Lines of code

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

Vulnerability details

Impact

Delegation cannot be removed because there is no use of toTokenId. removeDelegation takes the tokenId of the NFT to remove delegation from (according to the comment), but it checks the checkpoints mapping with that tokenId rather than the toTokenId (can be obtained using delegates mapping as delegates[tokenId]).

Proof of Concept

Let’s say X delegates to Y. delegates[X] = Y checkpoints[Y][0] = Checkpoint(block, [X])

X wants to remove the delegation to Y so removeDelegation(X) is called.

1 checkpoint so far Checkpoint storage checkpoint = checkpoints[X][0]; removeElement(checkpoint.delegatedTokenIds, tokenId);

However, X was never placed as a key in the checkpoints mapping, therefore checkpoints[X][0] is a non-existent checkpoint. It was Y when checkpoints[Y][0] = Checkpoint(block, [X]) was performed.

As a result, checkpoint.delegatedTokenIds does not contain X so X cannot be removed from delegating to Y.

Note: Based on the implementation, it can be unclear to what the parameter tokenId is supposed to represent (whether it is the token being delegated or the token being delegated to). If it is NFT Y that is intended to be passed for removeDelegation, the function will still fail to removeDelegation from X as removeElement(checkpoint.delegatedTokenIds, tokenId); but tokenId=Y.

Tools Used

Remix

Recommended Mitigation Steps

If the tokenId of the token being delegated is passed, then change:

 uint256 nCheckpoints = numCheckpoints[tokenId];
 Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];

To

uint delegate = delegates[tokenId] // delegate = toTokenId
uint256 nCheckpoints = numCheckpoints[delegate];
Checkpoint storage checkpoint = checkpoints[delegate][nCheckpoints - 1];

Otherwise: If the function is intended to remove all tokens that are delegated to the provided tokenId, then modify the logic to remove all elements in the delegated tokens list, rather than simply removing tokenId.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L203-L271

Vulnerability details

Impact

When ETH (msg.value) is sent as part of a call to fillAsk, the msg.value is checked to be greater than or equal to the "total value of one NFT * total number of NFTs + any extra payment to be given". However, if a user on accident or intentionally sends more than needed because they expected the excess ETH to be refunded, they will lose their assets.

Proof of Concept

Test placed in GolomTrader.specs.ts. The taker starts off with 10,000 ETH, sends 5,000 ETH to fillAsk, and is left with less than 5,000 ETH although the order price was a fraction of the amount sent.

it.only('excess not refunded', async () => {
            let exchangeAmount = ethers.utils.parseEther('1'); // cut for the exchanges
            let prePaymentAmt = ethers.utils.parseEther('0.25'); // royalty cut
            let totalAmt = ethers.utils.parseEther('10');
            let tokenId = await testErc721.current();

            const takerBalanceBefore = await ethers.provider.getBalance(taker.address);
            // Taker starts off with 10000 ETH
            expect(takerBalanceBefore).to.be.equals(utils.parseEther('10000'));

            const order = {
                collection: testErc721.address,
                tokenId: tokenId,
                signer: await maker.getAddress(),
                orderType: 0,
                totalAmt: totalAmt,
                exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
                prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
                isERC721: true,
                tokenAmt: 1,
                refererrAmt: 0,
                root: '0x0000000000000000000000000000000000000000000000000000000000000000',
                reservedAddress: constants.AddressZero,
                nonce: 0,
                deadline: Date.now() + 100000,
                r: '',
                s: '',
                v: 0,
            };

            let signature = (await maker._signTypedData(domain, types, order)).substring(2);

            order.r = '0x' + signature.substring(0, 64);
            order.s = '0x' + signature.substring(64, 128);
            order.v = parseInt(signature.substring(128, 130), 16);

            expect(utils.parseEther('5000')).to.be.gt(totalAmt);

            await golomTrader.connect(taker).fillAsk(
                order,
                1,
                '0x0000000000000000000000000000000000000000',
                {
                    paymentAmt: prePaymentAmt,
                    paymentAddress: await governance.getAddress(),
                },
                receiver,
                {
                    value: utils.parseEther('5000'), // clearly exceeds order amount
                }
            );

            const takerBalanceAfter = await ethers.provider.getBalance(taker.address);
            // Excess is not refunded
            expect(takerBalanceAfter).to.be.lt(utils.parseEther('5000'));
        });

Tools Used

Hardhat, Vscode

Recommended Mitigation Steps

Insert logic so that any excess ETH in fillAsk is returned to the sender. Another option is to use a strict equality against the msg.value.

• Compiler version specifications are not static, it is generally a best practice to select a specific compiler version. Otherwise, a known vulnerable solidity version can be used: GolomToken.sol::2 => pragma solidity ^0.8.11;

• Should use _ prepended to all private/internal functions. Functions missing a pretending _:

GolomTrader.sol

hashPayment, payEther

TokenUriHelper.sol

encode, toString

VoteEscrowDelegation.sol

removeElement

• snake_case is often used in place of camelCase:

VoteEscrowCore.sol

_supply_at, _find_block_epoch, increase_unlock_time, increase_amount, create_lock, create_lock_for, _create_lock, deposit_for, block_number, _deposit_for, locked__end, user_point_history__ts, get_last_user_slope.

• In the GolemTrader.sol contract, variables like status and orderType should use enumerations. status == 3 and orderType == 0 is not particularly readable/discoverable.

• VoteEscrowDelegation.sol transferFrom never used.

• In the VoteEscrowDelegation.sol contract, event DelegateVotesChanged(address indexed delegate, uint256 previousBalance, uint256 newBalance) is never used.

• Golom developers might consider adding in ETH and ERC20 rescue functions to the GolomTrader and GolomRewardDistributor contracts, in case any tokens/ether were to be errantly sent to these contracts.

• Golom developers might consider adding require(tokenId != toTokenId) in the delegate function in VoteEscrowDelegation.sol to prevent users from delegating a token to itself.

• Some public functions could be external:

fillBid(GolomTrader.Order,uint256,address,GolomTrader.Payment) should be declared external: - GolomTrader.fillBid(GolomTrader.Order,uint256,address,GolomTrader.Payment) (contracts/core/GolomTrader.sol#279-308) cancelOrder(GolomTrader.Order) should be declared external: - GolomTrader.cancelOrder(GolomTrader.Order) (contracts/core/GolomTrader.sol#312-317) fillCriteriaBid(GolomTrader.Order,uint256,uint256,bytes32[],address,GolomTrader.Payment) should be declared external: - GolomTrader.fillCriteriaBid(GolomTrader.Order,uint256,uint256,bytes32[],address,GolomTrader.Payment) (contracts/core/GolomTrader.sol#334-368) addFee(address[2],uint256) should be declared external: - RewardDistributor.addFee(address[2],uint256) (contracts/rewards/RewardDistributor.sol#98-138) traderClaim(address,uint256[]) should be declared external: - RewardDistributor.traderClaim(address,uint256[]) (contracts/rewards/RewardDistributor.sol#141-152) exchangeClaim(address,uint256[]) should be declared external: - RewardDistributor.exchangeClaim(address,uint256[]) (contracts/rewards/RewardDistributor.sol#155-166) multiStakerClaim(uint256[],uint256[]) should be declared external: - RewardDistributor.multiStakerClaim(uint256[],uint256[]) (contracts/rewards/RewardDistributor.sol#172-210) stakerRewards(uint256) should be declared external: - RewardDistributor.stakerRewards(uint256) (contracts/rewards/RewardDistributor.sol#215-250) traderRewards(address) should be declared external: - RewardDistributor.traderRewards(address) (contracts/rewards/RewardDistributor.sol#254-265) exchangeRewards(address) should be declared external: - RewardDistributor.exchangeRewards(address) (contracts/rewards/RewardDistributor.sol#269-280)

Variables should not be initialized to defaults (uint256 default is 0): GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) { RewardDistributor.sol::142 => uint256 reward = 0; RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol::156 => uint256 reward = 0; RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol::175 => uint256 reward = 0; RewardDistributor.sol::176 => uint256 rewardEth = 0; RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) { RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol::222 => uint256 reward = 0; RewardDistributor.sol::223 => uint256 rewardEth = 0; RewardDistributor.sol::226 => for (uint256 index = 0; index < epoch; index++) { RewardDistributor.sol::257 => uint256 reward = 0; RewardDistributor.sol::258 => for (uint256 index = 0; index < epoch; index++) { RewardDistributor.sol::272 => uint256 reward = 0; RewardDistributor.sol::273 => for (uint256 index = 0; index < epoch; index++) { VoteEscrowCore.sol::745 => for (uint256 i = 0; i < 255; ++i) { VoteEscrowCore.sol::1044 => for (uint256 i = 0; i < 128; ++i) { VoteEscrowCore.sol::1115 => for (uint256 i = 0; i < 128; ++i) { VoteEscrowCore.sol::1167 => for (uint256 i = 0; i < 255; ++i) { VoteEscrowCore.sol::1211 => uint256 dt = 0; VoteEscrowDelegation.sol::147 => uint256 lower = 0; VoteEscrowDelegation.sol::170 => uint256 votes = 0; VoteEscrowDelegation.sol::171 => for (uint256 index = 0; index < delegated.length; index++) { VoteEscrowDelegation.sol::188 => uint256 votes = 0; VoteEscrowDelegation.sol::189 => for (uint256 index = 0; index < delegatednft.length; index++) {

Length of array should be computed outside of for-loop: GolomTrader.sol::415 => for (uint256 i = 0; i < proof.length; i++) { RewardDistributor.sol::143 => for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol::157 => for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol::180 => for (uint256 tindex = 0; tindex < tokenids.length; tindex++) { RewardDistributor.sol::183 => for (uint256 index = 0; index < epochs.length; index++) { TokenUriHelper.sol::13 => uint256 len = data.length; VoteEscrowCore.sol::607 => if (reason.length == 0) { VoteEscrowDelegation.sol::99 => require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more'); VoteEscrowDelegation.sol::171 => for (uint256 index = 0; index < delegated.length; index++) { VoteEscrowDelegation.sol::189 => for (uint256 index = 0; index < delegatednft.length; index++) { VoteEscrowDelegation.sol::199 => for (uint256 i; i < _array.length; i++) { VoteEscrowDelegation.sol::201 => _array[i] = _array[_array.length - 1];

!= is more efficient than > 0 for uint comparisons: GolomTrader.sol::152 => if (payAmt > 0) { GolomTrader.sol::250 => if (o.refererrAmt > 0 && referrer != address(0)) { GolomTrader.sol::387 => if (o.refererrAmt > 0 && referrer != address(0)) { RewardDistributor.sol::124 => if (previousEpochFee > 0) { VoteEscrowCore.sol::579 => return size > 0; VoteEscrowCore.sol::704 => if (old_locked.end > block.timestamp && old_locked.amount > 0) { VoteEscrowCore.sol::708 => if (new_locked.end > block.timestamp && new_locked.amount > 0) { VoteEscrowCore.sol::727 => if (_epoch > 0) { VoteEscrowCore.sol::855 => // value == 0 (extend lock) or value > 0 (add to lock or extend lock) VoteEscrowCore.sol::927 => require(_value > 0); // dev: need non-zero value VoteEscrowCore.sol::928 => require(_locked.amount > 0, 'No existing lock found'); VoteEscrowCore.sol::944 => require(_value > 0); // dev: need non-zero value VoteEscrowCore.sol::981 => assert(_value > 0); // dev: need non-zero value VoteEscrowCore.sol::982 => require(_locked.amount > 0, 'No existing lock found'); VoteEscrowCore.sol::997 => require(_locked.amount > 0, 'Nothing is locked'); VoteEscrowDelegation.sol::78 => if (nCheckpoints > 0) { VoteEscrowDelegation.sol::103 => if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) { VoteEscrowDelegation.sol::119 => return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray;

Shortening revert strings to 32 bytes can decrease gas costs: TokenUriHelper.sol::72 TokenUriHelper.sol::76 TokenUriHelper.sol::78 TokenUriHelper.sol::84 TokenUriHelper.sol::86 TokenUriHelper.sol::92 TokenUriHelper.sol::94 TokenUriHelper.sol::100 TokenUriHelper.sol::102 TokenUriHelper.sol::112 VoteEscrowCore.sol::589 VoteEscrowCore.sol::625

Switching from division/multiplication to right-shift/left-shift can save gas: TokenUriHelper.sol::72 VoteEscrowCore.sol::296 => uint256 internal constant MAXTIME = 4 * 365 * 86400; VoteEscrowCore.sol::297 => int128 internal constant iMAXTIME = 4 * 365 * 86400; VoteEscrowCore.sol::1049 => uint256 _mid = (_min + _max + 1) / 2; VoteEscrowCore.sol::1120 => uint256 _mid = (_min + _max + 1) / 2; VoteEscrowDelegation.sol::150 => uint256 center = upper - (upper - lower) / 2; // ceil, avoiding overflow

VoteEscrowDelegation.sol::199 => In the removeElement for-loop, instead of i++ use unchecked {++i} in the body of the loop to save on overflow checks. We know there is a limit of 500 on delegatedTokenIds and that's all the removeElement function is ever used on. Additionally, the _element and i (index) variables can be declared as uint16 since we know there is a limit of 500 on the size of delegatedTokenIds.

GolomTrader::415 => The for-loop in _verifyProof can be more gas efficient by incrementing via ++i.