Golom contest - 0xsanson's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L300

Vulnerability details

Impact

Starting from ve == 0, we call addVoteEscrow.

    function addVoteEscrow(address _voteEscrow) external onlyOwner {
        if (address(ve) == address(0)) {
            ve = VE(pendingVoteEscrow);
        } else {
            voteEscrowEnableDate = block.timestamp + 1 days;
            pendingVoteEscrow = _voteEscrow;
        }
    }

The function sets ve to pendingVoteEscrow which is also address(0). There's no other way to change pendingVoteEscrow, so ve will always be unset.

Without ve it's impossible to claim staker rewards.

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L300

Recommended Mitigation Steps

Clearly, ve = VE(_voteEscrow); at line 300.

Lines of code

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

Vulnerability details

Impact

It's impossible to delegate to a token, because the first delegation will always revert.

Consider there's no delegation so far, meaning that every token will have numCheckpoints[token] = 0. When calling delegate, we will have nCheckpoints = 0 and the call will revert in _writeCheckpoint since nCheckpoints - 1 underflows.

    function delegate(uint256 tokenId, uint256 toTokenId) external {
    	...
        uint256 nCheckpoints = numCheckpoints[toTokenId];

        if (nCheckpoints > 0) {
        	...
        } else {
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
            _writeCheckpoint(toTokenId, nCheckpoints, array);
        }
    }
    
    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];  //@audit Reverts here
        
        ...
    }

Proof of Concept

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

Recommended Mitigation Steps

Instead of

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

write something like:

Checkpoint memory oldCheckpoint;
if (nCheckpoints > 0) oldCheckpoint  = checkpoints[toTokenId][nCheckpoints - 1];

Lines of code

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

Vulnerability details

Impact

It's possible to call delegate(tokenId1, tokenId2) using the same token tokenId1 multiple times. The previous delegated value isn't deleted, and all delegations stack up.

This way tokenId2 will have all these tokens as delegated:

checkpoints[tokenId2][nCheckpoints-1].delegatedTokenIds = [tokenId1, tokenId1, ...]

When calculating the votes for this token, there's no check that the delegated tokens are different, so the values all sum together.

Proof of Concept

Alice has two tokens. She calls delegate(tokenId1, tokenId2) multiple times, accruing the voting power of tokenId2 over her true token balance. During a governance proposal, GovernorAlpha#castVote will call VoteEscrow#getPriorVotes getting an artificially high voting power. She can use this power to pass whatever proposal she likes.

Recommended Mitigation Steps

Consider adding a require(delegates[tokenId] == address(0)) in the function delegate. Changing a delegation this way requires the user to call removeDelegation beforehand.

Lines of code

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

Vulnerability details

Impact

In VoteEscrowDelegation.sol, function _transferFrom will attempt to remove the delegations to the token transferred.

// remove the delegation
this.removeDelegation(_tokenId);

However this external call will change msg.sender to address(this), reverting the call when checking the owner

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 nCheckpoints = numCheckpoints[tokenId];
        Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        _writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

Also, even if the call was made internal, another revert is possible: if the token doesn't have any checkpoints yet, nCheckpoints - 1 will revert for underflow.

Proof of Concept

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

Recommended Mitigation Steps

1. Change removeDelegation to public, and write removeDelegation(_tokenId) instead of this.removeDelegation(_tokenId);
2. Add a if (nCheckpoints == 0) return; in removeDelegation before the possible underflow.

Lines of code

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

Vulnerability details

Impact

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

Inside the first if, we are writing to memory instead of storage. This means that if nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number, the function will absolutely do nothing.

Proof of Concept

Alice flashloans tokenId1. She then calls delegate(tokenId1, tokenId2) where tokenId2 is a token she owns permanently. When she returns the first token, there will be an attempt to remove tokenId1 from the delegates of tokenId2 (not considering other issues in the removeDelegation function). However, since oldCheckpoint.fromBlock == block.number, _writeCheckpoint will fail to change the state.

Using this trick, Alice is able then to bypass any removeDelegation, leading to possible governance attacks.

Also the contract state will be messed up going forward, since the checkpoint of tokenId2 will have tokenId1, but delegates[tokenId1] is deleted. If delegates was used as check against delegating the same token multiple times, this exploit can be used to continue adding the same token as delegate, increasing limitlessly the governance power of an exploiter.

Recommended Mitigation Steps

Checkpoint memory oldCheckpoint to storage.

Lines of code

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

Vulnerability details

Impact

VoteEscrow tokens are used as voting tokens for a GovernorAlpha governance. It shouldn't be possible to modify an old (meaning for blocks older than block.number) checkpoint, otherwise it's possible to buy tokens just to vote for a proposal and then sell them.

In delegate function however there's such a bug.

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

_writeCheckpoint correctly makes a new checkpoint for the correct block number, but before that the code adds a new token to the last checkpoint, which has an older block number.

Proof of Concept

A new governance proposal started. Alice has already a token id1. She can buy (and later sell) a token id2. Calling delegate(id2, id1) she adds id2 to her delegations, and she's able to vote also using its balance.

Let's say she also has a token id3 before the proposal started. She can call delegate(id2, id3) (deleting id2 delegates beforehand) and id2 will also count when she votes using id3 (it's basically a double count for id2). The process can of course be repeated with multiple tokens, leading to abusers getting way more voting powers than normal.

Recommended Mitigation Steps

Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1]; to memory.

Lines of code

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

Vulnerability details

Impact

The function removeDelegation(tokenId) currently tries to remove tokenId from the list of tokens delegated to itself.

The correct behavior would be to get which token tokenId is delegated to, and remove tokenId from the delegations to that token.

Since removeDelegation is important in resetting delegations during token transfers, this malfunctions allows some users to gain more governance votes.

Proof of Concept

Alice flashloans (or simply loans) some tokens with high balance and, before returning them, she delegate their voting power to another token she controls. Since removeDelegation malfunctions, she keeps the votes of all these tokens, leading to a possible governance attack.

Recommended Mitigation Steps

Use delegates[tokenId] to get the correct token from which getting the checkpoint.

    function removeDelegation(uint256 tokenId) external {
        require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');
        uint256 toTokenId = delegates[tokenId];      //@audit Add this
        //uint256 nCheckpoints = numCheckpoints[tokenId];
        uint256 nCheckpoints = numCheckpoints[toTokenId];
        //Checkpoint storage checkpoint = checkpoints[tokenId][nCheckpoints - 1];
        Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1]; 
        removeElement(checkpoint.delegatedTokenIds, tokenId);
        //_writeCheckpoint(tokenId, nCheckpoints, checkpoint.delegatedTokenIds);
        _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    }

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

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

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

[L5] chainId constant

In GolomTrader, EIP712_DOMAIN_TYPEHASH is calculated during deployment and it's considered immutable. However the parameter chainId can change during a hard-fork of the blockchain, leading to replay attacks between the two child chains.

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

Recommendations

Consider checking chainId against a deployment-fixed value, so in case it's different the appropriate EIP712_DOMAIN_TYPEHASH can be calculated.

Lines of code

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

Vulnerability details

Impact

When delegating voting power to a user, the contract stores a checkpoint with all delegated tokens in an array uint256[] delegatedTokenIds.

When we want to delegate a new tokenId to toTokenId, the contract requires that the array length stays under 500:

require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');

This means that it's possible for someone to delegate 500 tokens to a victim and stop further delegations. Notice that there's no floor cost for this attack beside gas costs (since MIN_VOTING_POWER_REQUIRED = 0).

Proof of Concept

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

Recommended Mitigation Steps

Consider changing MIN_VOTING_POWER_REQUIRED to a relatively high value.

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

Lines of code

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

Vulnerability details

Impact

In VoteEscrowDelegation.sol, function _transferFrom will attempt to remove the delegations to the token transferred.

// remove the delegation
this.removeDelegation(_tokenId);

if the token doesn't have any checkpoints yet, nCheckpoints - 1 will revert for underflow.

Proof of Concept

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

Recommended Mitigation Steps

Add a if (nCheckpoints == 0) return; in removeDelegation before the possible underflow.

[L1] Extra msg.value is stuck in contract when fillAsk

When filling a "ask", the filler provides ETH by sending msg.value along the call. This value is checked to be greater or equal than the order value:

// attached ETH value should be greater than total value of one NFT * total number of NFTs + any extra payment to be given
require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

However there's no refund for sending a greater value, and the extra quantity is stuck in the contract.

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

Recommendations

Since the order value doesn't change in time, consider using == instead of >= in the requirement before. Otherwise refund the extra value to msg.sender.

[L2] Unsafe ERC721 transferFrom

GolomTrader uses transferFrom to move ERC721 tokens, instead of safeTransferFrom. If a token recipient is a contract, it's possible that the NFT will be lost.

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

Recommendations

Consider using safeTransferFrom unless gas savings is prioritized.

[L3] removeDelegation should also delete delegates

In VoteEscrowDelegation.sol, with function delegate we write to the mapping delegates.

    /// @notice Delegate of the specific token
    mapping(uint256 => uint256) public delegates;
    
    function delegate(uint256 tokenId, uint256 toTokenId) external {
    	...
        delegates[tokenId] = toTokenId;
        ...
    }

There's a function called removeDelegation which removes the checkpoint but doesn't touch delegates.

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

Recommendations

It would make sense to reset also delegates during removeDelegation.

[L4] ETH transfer using transfer instead of call

In function payEther, ETH transfers are done using transfer. This makes a message with a fixed 2300 gas, it's possible that a contract recipient isn't able to execute a fallback function, and revert the whole transaction.

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

Recommendations

Consider using a low-level call. Re-entrancy shouldn't be an issue given the nonReentrant modifiers.

[L5] chainId constant

In GolomTrader, EIP712_DOMAIN_TYPEHASH is calculated during deployment and it's considered immutable. However the parameter chainId can change during a hard-fork of the blockchain, leading to replay attacks between the two child chains.

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

Recommendations

Consider checking chainId against a deployment-fixed value, so in case it's different the appropriate EIP712_DOMAIN_TYPEHASH can be calculated.