Golom contest - scaraven's results

Lines of code

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

Vulnerability details

Impact

In _writeCheckpoint in VoteEscrowDelegate, the contract tries to access the previous checkpoint at index nCheckpoints - 1. However, initially, when a user is delegated to this value is 0 thereby creating an arithmetic underflow and reverts the transaction. This prevents anyone from being delegated and because GovernerBravo.sol only counts delegated votes (and does not include the token vote itself), it would be impossible for anyone to create or vote on proposals.

Proof of Concept

1. A user with tokenId 1 delegates votes to token 2 by calling delegate()
2. As nCheckpoints == 0, a new dynamic array is created and passed to _writeCheckpoints
3. The contract performs the calculation nCheckpoints - 1 which reverts with an arithmetic underflow

Tools Used

VS Code

Recommended Mitigation Steps

Change _writeCheckpoint from

    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];


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

to

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


        if (nCheckpoints > 0) {
            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;
              }
            
        } 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/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L389-L394 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L396-L399

Vulnerability details

Impact

When calculating protocolFee in _settleBalances() in GolomTrader, the protocolFee is multiplied by amount twice which can drastically increase the fees that a user receives.

This issue counts as loss of funds and breaks a major selling point of the protocol (low fees) which I believe is high severity.

Proof of Concept

1. A sender wants to sell 10 ERC1155 tokens to an off chain signer for 50 ETH each
2. The sender executes fillBid and _settleBalances is called
3. protocolFee is calculated as 50 * 50 / 10000 * 10 = 2.5 ETH
4. Assuming all paymentAmt are zero, the sender receives (50 - 2.5)*10 = 475 ETH
5. The sender loses 5% instead of 0.5% with 22.5 ETH being left in the contract

Tools Used

VS Code

Recommended Mitigation Steps

Change the payments in _settleBalances from

            payEther(
                (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
                msg.sender
            );

to

            payEther(
                (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt - protocolfee,
                msg.sender
            );

Lines of code

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

Vulnerability details

Impact

In delegate() of VoteEscrowDelegate, no check is made that the token has been delegated to another token or delegated multiple times. This allows a user with a token to delegate to itself an arbitrary number of times so that the user in effect controls over 50% of votes. The user can then create a new malicious proposal on GovernerBravo to upgrade to a malicious contract and empty all funds from the protocol etc...

Proof of Concept

1. A user owns a token with tokenId 1 with voting power of 0.2% of the entire voting power
2. They then delegate that token to themselves 500 times so that they have voting power equal to the total voting power
3. The user can then propose and execute any malicious proposal on GovernerBravo

Tools Used

VS Code

Recommended Mitigation Steps

Add a check which makes sure that a token has not already been delegated e.g. In delegate()

require(delegates[tokenId] == 0, "Already delegated");

Make sure that when you remove delegations that you reset this value

Lines of code

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

Vulnerability details

Impact

When the 1 billion supply has been reached for the GOLOM token, addFee() in RewardDistributor will automatically return to the start without updating any trades, however will still accept fees being sent from GolomTrader. These fees however cannot be claimed by anyone, thereby permanently locking these fees into the contract.

Tools Used

VS Code

Recommended Mitigation Steps

Consider preventing fees from being sent to the reward distributor after the max supply has been reached or allowing fees to be sent to governance or some other privileged role.

Lines of code

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

Vulnerability details

Impact

In _writeCheckpoint, when the oldCheckpoint is accessed, it is stored in memory so any changes to that variable is not permanently stored. This means that if a user transfers a token to someone else, if a delegation is made in the same block then the delegations are not removed. This allows the new owner to essentially keep all the delegations (without the delegators consent) and use them for malicious purposes.

I believe this issue is medium severity (as it requires frontrunning) however can cause a severe breach in trust between delegators and the delegated.

Proof of Concept

1. Bob transfers a token to alice (this can be mediated via a marketplace where alice bought the token off bob)
2. Before the transfer is made, alice frontruns the tx and delegates a random token to bob's token
3. As a token has been delegated in the same block that bob's token is being transferred in, the if statement:

if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) {

passes 4. The delegatedIds in storage is unchanged thereby allowing alice to keep all of bob's delegations

Tools Used

VS Code

Recommended Mitigation Steps

In _withdrawCheckpoint change

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

to

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

Lines of code

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

Vulnerability details

Impact

As of now, removeDelegation removes the delegation of a token to itself, however leaves all delegators in place. This can be very dangerous because if this token is transferred to a malicious user (for example the malicious user buys the token from the original user on a secondary market) then the token can be used to propose and vote for malicious upgrades/proposals which can hijack the entire protocol. This essentially allows an attacker to buyout votes without the explicit consent of the original delegator.

Proof of Concept

1. Alice owns a token with 10% of the voting power of the protocol
2. Bob buys the token from alice. Bob still manages to keep all the delegations of that token
3. Bob can now try to propose and vote for a malicious upgrade which steals all reward tokens, nft's, etc...

Tools Used

VS Code

Recommended Mitigation Steps

Consider changing removeDelegation so that it removes all delegations. This can be done by passing an empty dynamic array for _writeCheckpoint()

Lines of code

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

Vulnerability details

Impact

An issue from a past Code4Arena contest here describes the issue with transfer() very well.

Tools Used

VS Code

Recommended Mitigation Steps

Use call() instead of transfer() and make sure to check that the call did succeed

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L873-L876 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L883-L886 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L894 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L538 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1008

Vulnerability details

Impact

A malicious voter can arbitrarily increase the number of attachments or set the voted status of a token to true. This prevents the token from being withdrawn, merged or transfered thereby locking the tokens into the contract for as long as the voter would like.

I submitted this is as a medium severity because it has external circumstances (a malicious voter) however has a very high impact if it does occur.

Proof of Concept

1. A user creates a lock for their token and deposits it into the VoteEscrowDelegate/Core contract.
2. The malicious voter then calls either voting() or attach() thereby preventing the user withdrawing their token after the locked time bypasses

Tools Used

VS Code

Recommended Mitigation Steps

I have not seen any use of voting() or attach() in any of the other contracts so it may be sensible to remove those functions altogether. On the other hand, setting voter to be smart contract which is not malicious offsets this problem.

Lines of code

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

Vulnerability details

Impact

Currently, in fillAsk the check to make sure that sufficient ETH has been supplied uses >= however any excess ethereum is not sent back to the user. Therefore, if a user sends too much ETH to the contract when buying an NFT will permanently lose those funds.

I believe this is medium severity as it requires external circumstances (users sends too much ETH) but causes permanent locking and loss of funds. Additionally, this problem can be very easily avoided.

Tools Used

VS Code

Recommended Mitigation Steps

In fillAsk() change

        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

to

        require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

Issue 1 Use of magic values

Magic values make the code less clear and can be especially annoying when you want to update them during development. Consider storing magic values as variables e.g.

uint256 public constant GENESIS_REWARD = 62_500_000 * 1e18;
...
        _mint(_rewardDistributor, GENESIS_REWARD);

instead of

        _mint(_rewardDistributor, 62_500_000 * 1e18);

Occurrences: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L44 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L52 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L60 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L100 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L120-L121 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L286 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L302 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L212 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L254-L255 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L263 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L269 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L449 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L99

Issue 2 increase_amount() in VoteEscrowCore is obsolete

increase_amount() makes a check to make sure that the sender is the owner of the token. Other than this check, increase_amount() performs the exact same operations as deposit_for which does not have the authentication check.

Consider removing increase_amount() Occurrences: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L977

Issue 3 Obsolete code in validateOrder() from GolomTrader

validateOrder() checks whether the signer of the order hash is the same as o.signer twice in the same function, thereby rendering the second check obsolete. Consider removing one of these checks

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

Issue 4 Number of reward tokens can become larger than 1 billion

addFees makes sure that current supply is less than 1 billion however does not account for new reward tokens minted later on. This can cause the supply to become larger than 1 billion

Consider adding a check which makes sure that the supply is less than 1 billion after minting reward tokens

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

Issue 5 Incorrect code for removeDelegationByOwner() in VoteEscrowDelegate

Currently, this code is commented out, so I feel like low severity is suitable however it may be a medium as the sponsor has told me that it is in scope.

removeDelegationByOwner() is supposed to remove the owner from delegation to another token however instead would remove the delegated token from own delegation

Consider changing

removeElement(checkpoint.delegatedTokenIds, delegatedTokenId);

to

removeElement(checkpoint.delegatedTokenIds, ownerTokenId);

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

Issue 6 Ensure that Governance can not submit zero addresses for privileged roles

Currently, if governance update an address to address(0) then after the initial time period, governance can instantly set a new address which may be malicious.

Consider adding a check to make sure that the new address is non zero e.g.

require(_addr != 0, "Non zero address");

Occurrences: https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L66-L67 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L445-L446 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L299-L300

Issue 7 Inconsistent checks on totalAmt made in fillBid and fillCriteriaBid

Currently fillBid() executes:

        require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt
        ); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

while fillCriteriaBid() executes:

        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

None of these checks account for the protocol fee and therefore if totalAmt is too low then it can cause either the transaction to revert suddenly or for any excess ethereum stored in the contract to be funnelled away.

Consider changing the checks to the following:

 require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + o.totalAmt * 50 / 10000) * amount + p.paymentAmt
             );