Golom contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L71-L89

Vulnerability details

Impact

There are no checks that the lock NFT has not already been delegated. Therefore, an attacker can essentially multiply their voting power by as many times as they call delegate().

Proof of Concept

Taking a look at the delegate() function:

    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');


        delegates[tokenId] = toTokenId;
        uint256 nCheckpoints = numCheckpoints[toTokenId];


        if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(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);
    }

Calling delegate() simply pushes the user's tokenId into the array for the toTokenId via _writeCheckpoint(). There are no checks that the tokenId has not been previously delegated.

The calculation for voting power looks as such:

    function getVotes(uint256 tokenId) external view returns (uint256) {
        uint256[] memory delegated = _getCurrentDelegated(tokenId);
        uint256 votes = 0;
        for (uint256 index = 0; index < delegated.length; index++) {
            votes = votes + this.balanceOfNFT(delegated[index]);
        }
        return votes;
    }

Firstly, the delegated tokenIds are returned via _getCurrentDelegated():

    function _getCurrentDelegated(uint256 tokenId) internal view returns (uint256[] memory) {
        uint256 nCheckpoints = numCheckpoints[tokenId];
        uint256[] memory myArray;
        return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray;
    }

These delegated tokens are then looped through, incrementing the total voting power by the voting power of the individual delegated tokenId.

• A user can create a lock that generates an arbitrary 10 voting power.
• The user then delegates this lock 500 times which is the maximum limit for a single delegatee.
• The user now has 5000 voting power attributed to their delegatee.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a check that the tokenId is not currently delegated. If so, revert.

Lines of code

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

Vulnerability details

Impact

The payEther() function uses the transfer() function which forwards a fixed 2300 gas to the recipient. This can lead to reverts in a couple of scenarios:

• If the recipient attempts to perform follow-up logic/calls and runs out of gas
• If the recipient is a smart contract wallet such as a Gnossis Safe
• If gas costs for certain OP codes increase in the future.

Proof of Concept

Nice article here (credit to Kenzo): https://help.gnosis-safe.io/en/articles/5249851-why-can-t-i-transfer-eth-from-a-contract-into-a-safe

The payEther() function in question:

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Use call() over transfer() to mitigate this potential issue.

Judge has assessed an item in Issue #727 as Medium risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

Similar to a previous submission, there are no checks preventing against delegating the same lock NFT multiple times. This opens an avenue to an expensive but potentially profitable griefing attack where the malicious user fills the victim's delegated token array with minimum voting power. The attacker can ensure that a delegatee has 0 voting power.

Proof of Concept

Taking a look at the delegate() function below, there are no checks that a lock NFT has not already been delegated. Therefore, an attacker can delegate their token with minimum voting power (threshold initialized with value 0) to the victim.

    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');


        delegates[tokenId] = toTokenId;
        uint256 nCheckpoints = numCheckpoints[toTokenId];


        if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(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);
    }

There is a limit of 500 delegated tokens per delegatee. Therefore, the attacker can ensure minimum voting power if they delegate a worthless token 500 times to the victim:

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

A more likely scenario would be as follows:

• A proposal is live.
• Users delegate their voting power to addresses of their choosing.
• A and B are around the same voting power.
• A and B both have 400 delegatees.
• Malicious address A delegates minimum voting power 100 times to fill B's array to 500.
• Address A can self-delegate just a bit more to obtain more voting power.

Tools Used

Manual review.

Recommended Mitigation Steps

Firstly, removing the ability to delegate the same lock NFT would make this griefing attack much more expensive. Even if that is patched, a griefing attack is still possible by simply creating more locks and delegating them all once.

I believe that removing the 500 delegated token limit would prove to mitigate this issue.

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Exceeding value is not refunded and will get stuck in contract

User can mistakenly input and extra 0 and send 100 tokens instead of 10. The contract doesn't refund the excess and has no way to withdraw() such funds. Recommend refunding excess to user. 1 instance of this issue has been found:

[L-01] GolomTrader.sol#L217-L218

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

[L-02] Floating Pragma

A floating pragma might result in contract being tested/deployed with different compiler versions possibly leading to unexpected behaviour. 1 instance of this issue has been found:

[L-02] GolomToken.sol#L2-L3

pragma solidity ^0.8.11;

[L-03] Unsafe transfer()/transferFrom()

Some tokens might return 0 instead of reverting on failure. If return value of transfer()/transferFrom() is not checked the transaction might silently fail. 4 instances of this issue have been found:

[L-03] RewardDistributor.sol#L208-L209

        rewardToken.transfer(tokenowner, reward);

[L-03b] RewardDistributor.sol#L151-L152

        rewardToken.transfer(addr, reward);

[L-03c] GolomTrader.sol#L301-L302

            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

[L-03d] GolomTrader.sol#L236-L237

            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

[L-04] Missing zero address check might cause loss of funds

It's a good idea to add a require statement checking that the input address is not address(0) to ensure against mistaken function calls. 1 instance of this issue has been found:

[L-04] GolomToken.sol#L50-L51

        _mint(_airdrop, 150_000_000 * 1e18);

[L-05] Voter transfer should be two-step process

Since the transfer of the voter role is one-way in that only the current voter can set the new voter, it is recommended to employ a two-step role transfer process. This involves setting a pending voter and accepting the change from the pending voter address. 1 instance of this issue has been found:

[L-05] VoteEscrowCore.sol#L868-L872

    function setVoter(address _voter) external {
        require(msg.sender == voter);
        voter = _voter;
    }

[L-06] Unreachable code: if statement will never run as require will cause revert

The require statement will cause a revert if signaturesigner != o.signer. Therefore, the if statement that follows will never be true. 1 instance of this issue has been found:

[L-06] GolomTrader.sol#L177-L181

        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

[L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. 1 instance of this issue has been found:

[L-07] GolomTrader.sol#L175-L176

        bytes32 hash = keccak256(abi.encodePacked('\x19\x01', EIP712_DOMAIN_TYPEHASH, hashStruct));

[L-08] Empty fallback()/receive() function

If intended functionality is to make use of ETH the receive() function should implement some operation, if not it should revert the transaction. 2 instances of this issue have been found:

[L-08] GolomTrader.sol#L458-L461

    fallback() external payable {}

    receive() external payable {}

[L-08b] RewardDistributor.sol#L313-L315

    fallback() external payable {}

    receive() external payable {}

Non-critical Findings

[N-01] Function should return value

Providing a return value where improves code clarity and readbility. 1 instance of this issue has been found:

[N-01] RewardDistributor.sol#L98-L138

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
        //console.log(block.timestamp,epoch,fee);
        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
            // if supply is greater then a billion dont mint anything, dont add trades
            return;
        }

        // if 24 hours have passed since last epoch change
        if (block.timestamp > startTime + (epoch) * secsInDay) {
            // this assumes atleast 1 trade is done daily??????
            // logic to decide how much token to emit
            // emission = daily * (1 - (balance of locker/ total supply))  full if 0 locked and 0 if all locked
            // uint256 tokenToEmit = dailyEmission * rewardToken.balanceOf()/
            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining
            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();
            // deposit previous epoch fee to weth for distribution to stakers

            uint256 previousEpochFee = epochTotalFee[epoch];
            epoch = epoch + 1;
            rewardStaker[epoch] = stakerReward;
            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100; //@audit Rounding error?
            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;
            rewardToken.mint(address(this), tokenToEmit);
            epochBeginTime[epoch] = block.number;
            if (previousEpochFee > 0) {
                if (epoch == 1){
                    epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                    weth.deposit{value: address(this).balance}();  
                }else{
                    weth.deposit{value: previousEpochFee}();
                }
            }
            emit NewEpoch(epoch, tokenToEmit, stakerReward, previousEpochFee);
        }
        feesTrader[addr[0]][epoch] = feesTrader[addr[0]][epoch] + fee;
        feesExchange[addr[1]][epoch] = feesExchange[addr[1]][epoch] + fee;
        epochTotalFee[epoch] = epochTotalFee[epoch] + fee;
        return;
    }

[N-02] 255 should be written as type(uint<n>).max

Using magic numbers is not recommended. 1 instance of this issue has been found:

[N-02] VoteEscrowCore.sol#L745-L746

            for (uint256 i = 0; i < 255; ++i) {

[N-03] It is recommend to use scientific notation (1e18) instead of exponential (10**18)

Improves readbility. 1 instance of this issue has been found:

[N-03] RewardDistributor.sol#L48-L49

    uint256 constant dailyEmission = 600000 * 10**18;

[N-04] The use of magic numbers is not recommended

Consider setting constant numbers as a constant variable for better readability and clarity. 10 instances of this issue have been found:

[N-04] RewardDistributor.sol#L84-L85

        startTime = 1659211200;

[N-04b] VoteEscrowCore.sol#L308-L309

    mapping(uint256 => Point[1000000000]) public user_point_history; // user -> Point[user_epoch]

[N-04c] GolomTrader.sol#L269-L270

        distributor.addFee([o.signer, o.exchange.paymentAddress], ((o.totalAmt * 50) / 10000) * amount);

[N-04d] GolomTrader.sol#L263-L264

                (o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount,

[N-04e] GolomTrader.sol#L252-L260

            payEther(
                (o.totalAmt -
                    (o.totalAmt * 50) /
                    10000 -
                    o.exchange.paymentAmt -
                    o.prePayment.paymentAmt -
                    o.refererrAmt) * amount,
                o.signer
            );

[N-04f] GolomTrader.sol#L242-L243

        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));

[N-04g] GolomTrader.sol#L211-L214

        require(
            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
            'amt not matching'
        );

[N-04h] RewardDistributor.sol#L100-L101

        if (rewardToken.totalSupply() > 1000000000 * 10**18) {

[N-04i] GolomToken.sol#L52-L53

        _mint(_rewardDistributor, 62_500_000 * 1e18);

[N-04j] GolomToken.sol#L44-L45

        _mint(_airdrop, 150_000_000 * 1e18);

[N-05] File name doesn’t match contract name

The contract is titled VoteEscrowDelegation.sol but the contract is VoteEscrow.sol

Universal Description: The contract file name and contract name should be exactly the same. Please refactor. 1 instance of this issue has been found:

[N-05] VoteEscrowDelegation.sol#L20-L21

contract VoteEscrow is VoteEscrowCore, Ownable {

[N-06] Incomplete comments

Please ensure spec comments are provide a complete description of intended functionality 4 instances of this issue have been found:

[N-06] RewardDistributor.sol#L107-L108

            // this assumes atleast 1 trade is done daily??????

[N-06b] GolomTrader.sol#L160-L161

    ///      OrderStatus = 1 , if deadline has been

[N-06c] GolomToken.sol#L5

		/// @notice Explain to an end user what this does

[N-06d] RewardDistributor.sol#L110-L111

            // uint256 tokenToEmit = dailyEmission * rewardToken.balanceOf()/

[N-07] Solidity recommended practice to name functions in camel-case, not snake-case (many instances, find them)

Solidity style guide recommends to name functions as camelCase() instead of snake_case(). 1 instance of this issue has been found:

[N-07] VoteEscrowCore.sol#L833-L834

    function _deposit_for(

[N-08] Typos

Please fix typos. 3 instances of this issue have been found:

[N-08] RewardDistributor.sol#L154-L155

// Typo in "facilitated" and "their"
    // allows exchange that facilated the nft trades to claim there previous epoch rewards

[N-08b] RewardDistributor.sol#L168-L169

// Typo in "their"
    /// @dev allows VeNFT holders to claim there token and eth rewards

[N-08c] RewardDistributor.sol#L111-L112

// Typo in "is (are)" and "beginning"
            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining

[N-09] Constants should be written in all caps

Solidity style guide recommends to employ all capital letters for constant declarations. 1 instance of this issue has been found:

[N-09] RewardDistributor.sol#L57-L58

    uint256 constant secsInDay = 24 * 60 * 60;

[N-10] Misleading require message

In the case that a user attempts to take more than the maker is selling, the revert message does not make sense. 1 instance of this issue has been found:

[N-010] GolomTrader.sol#L227-L228

        require(amountRemaining >= amount, 'order already filled');

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

1 instance of this issue has been found:

[G-01] GolomTrader.sol#L415-L416

        for (uint256 i = 0; i < proof.length; i++) {

[G-02] Using prefix(++i) instead of postfix (i++) saves gas

It saves 6 gas per iteration. 1 instance of this issue has been found:

[G-02] GolomTrader.sol#L415-L416

        for (uint256 i = 0; i < proof.length; i++) {

[G-03] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

3 instances of this issue have been found:

[G-03] VoteEscrowCore.sol#L1044-L1045

        for (uint256 i = 0; i < 128; ++i) {

[G-03b] GolomTrader.sol#L415-L416

        for (uint256 i = 0; i < proof.length; i++) {

[G-03c] VoteEscrowCore.sol#L745-L746

            for (uint256 i = 0; i < 255; ++i) {

[G-04] Redundant fallback() function

receive() function is already present. If intended functionality is to make use of ETH the fallback() function should implement some operation. 2 instances of this issue have been found:

[G-04] RewardDistributor.sol#L313-L315

    fallback() external payable {}

    receive() external payable {}

[G-04b] GolomTrader.sol#L459-L461

    fallback() external payable {}

    receive() external payable {}

[G-05] Setting variable to default value is redundant

Setting variable to default value is redundant 11 instances of this issue have been found:

[G-05] VoteEscrowDelegation.sol#L50-L51

    uint256 public MIN_VOTING_POWER_REQUIRED = 0;

[G-05b] VoteEscrowCore.sol#L1211-L1212

        uint256 dt = 0;

[G-05c] VoteEscrowCore.sol#L1169-L1170

            int128 d_slope = 0;

[G-05d] VoteEscrowCore.sol#L1134-L1135

        uint256 d_t = 0;

[G-05e] VoteEscrowCore.sol#L1133-L1134

        uint256 d_block = 0;

[G-05f] VoteEscrowCore.sol#L1113-L1114

        uint256 _min = 0;

[G-05g] VoteEscrowCore.sol#L1042-L1043

        uint256 _min = 0;

[G-05h] VoteEscrowCore.sol#L1044-L1045

        for (uint256 i = 0; i < 128; ++i) {

[G-05i] VoteEscrowCore.sol#L745-L746

            for (uint256 i = 0; i < 255; ++i) {

[G-05j] GolomTrader.sol#L415-L416

        for (uint256 i = 0; i < proof.length; i++) {

[G-05k] RewardDistributor.sol#L45-L46

    uint256 public epoch = 0;

[G-06] Variable should be immutable

Variables set in the constructor that are not able to be changed by other functions should be set as immutable in order to save gas. 3 instances of this issue have been found:

[G-06] RewardDistributor.sol#L46

uint256 public startTime;

[G-06b] RewardDistributor.sol#L68-L69

    ERC20 public rewardToken;

[G-06c] RewardDistributor.sol#L69-L70

    ERC20 public weth;

[G-07] Use custom errors rather than revert()/require() strings to save gas

Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. 1 instance of this issue has been found:

[G-07] GolomTrader.sol#L211-L214

       require(
            o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt + (o.totalAmt * 50) / 10000,
            'amt not matching'
        );

[G-08] require()/revert() checks used multiples times should be turned into a function or modifier

Doing so increases code readability decreases number of instructions for the compiler. 2 instances of this issue have been found:

[G-08] RewardDistributor.sol#L292-L293

        require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

[G-08b] RewardDistributor.sol#L309-L310

        require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet');

[G-09] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operatio 1 instance of this issue has been found:

[G-09] RewardDistributor.sol#L58-L60

    mapping(address => mapping(uint256 => uint256)) public feesTrader; // fees accumulated by address of trader per epoch
    mapping(address => mapping(uint256 => uint256)) public feesExchange; // fees accumulated by exchange of trader per epoch
    

[G-010] Store the result of this calculation to avoid calculating 5 times

It is unnecessary to perform the same calculation many times. Calculate it once and store it in memory. Then read from memory. 1 instance of this issue has been found:

[G-010] GolomTrader.sol#L242-L243

        ((o.totalAmt * 50) / 10000) * amount

[G-011] bool costs more gas than setting uint(8) equal to 0

It costs less gas to set false states as 0 and true states as 1 in a uint(8). 1 instance of this issue has been found:

[G-011] GolomToken.sol#L20-L21

    bool public isAirdropMinted;
    bool public isGenesisRewardMinted;

[G-012] Struct could be packed more efficiently

It is possible to more efficiently order the variables of the struct in order to minimize the number of storage slots necessary to hold the data. Each storage slot can hold up to 32 bytes. 1 instance of this issue has been found:

[G-012] GolomTrader.sol#L47-L65

    struct Order {
        address collection; // NFT contract address
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
        Payment prePayment; // another payment , can be used for royalty, facilating trades
        bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
        uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
        uint256 refererrAmt; // amt to pay to the address that helps in filling your order
        bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
        address reservedAddress; // if not address(0) , only this address can fill the order
        uint256 nonce; // nonce of order usefull for cancelling in bulk
        uint256 deadline; // timestamp till order is valid epoch timestamp in secs
        uint8 v;
        bytes32 r;
        bytes32 s;
    }

[G-013] Repeated read from storage

Reading from storage is an expensive operation. Storing the first read in memory will save significant gas. 1 instance of this issue has been found:

[G-013] VoteEscrowCore.sol#L644-L651

        address owner = idToOwner[_tokenId];
        ...
        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);