Golom contest - ak1's results

Lines of code

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

Vulnerability details

Impact

Due to incorrect state variable update, VoteEscrow will not function as intended. All other functions calling the _writeCheckpoint function also will affect because of this.

Proof of Concept

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

In above function, the variable oldCheckpoint is stored in memory and then its used to update the delegatedTokenIds in state. Since oldCheckpoint is stored in memory, updating this will never update the state variable.

This will affect the entire VoteEscrow mechanism.

Tools Used

Manual code review. Discussed with Saruman | golom.io about my finding and confirmed.

Recommended Mitigation Steps

store the variable in storage and update it.

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

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

1. Suggestion to remove the Dead code, the second if condition never execute https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L177-L180

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

2. Use of proper function signature to calculate the keecak256 hash. Check for valid character (lower case or upper case letter) is suggested. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L116 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L132

3. Suggested to provide meaninful error messages in following line of codes when revert happens. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L213 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L217

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

4. If to is zero address, it is suggested to revert. I can see the comments but the code is not written in such way. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowCore.sol#L563-L570

    function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
      ) external {
    _transferFrom(_from, _to, _tokenId, msg.sender);
     }
   

5. It will be always suggested to check for valid contract address even if owner only allowed to do changes. Check following lines of codes where to validate the input. All other contracts can be considered for this change. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L58-L61

   function setMinter(address _minter) external onlyOwner { require(msg.sender != address(0)) add pendingMinter = _minter; minterEnableDate = block.timestamp + 1 days; }

   As shown above, add similar validation in following codes also. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444-L451

1. Gas reduction by updating the codes as shown below. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L168-L175

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

2. Gas reduction by updating the codes as shown below. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L185-L193

   function getPriorVotes(uint256 tokenId, uint256 blockNumber) public view returns (uint256 +votes) { add require(blockNumber < block.number, 'VEDelegation: not yet determined'); uint256[] memory delegatednft = _getPriorDelegated(tokenId, blockNumber); -uint256 votes = 0; remove for (uint256 index = 0; index < delegatednft.length; ++index-(++)) { use pre increment votes = votes + this.balanceOfAtNFT(delegatednft[index], blockNumber); } -return votes; remove }

3. Use pre-increment operator in following lines

   https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L199 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L258 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L273

   for (uint256 i; i < _array.length; ++i -(++)) use pre-increment

4. Re-arrange the variable declaration order and change variabe type to uint8 to save gas and storage. https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/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; the above removed variables can be brought down as shown below these two can be stored in single slot. this could save storage as well as gas +uint8 orderType; since it stores 0, 1, 2 , uint8 is sufficient +uint8 v; }