Golom contest - dipp's results

Lines of code

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

Vulnerability details

The delegate function in VoteEscrowDelegation.sol is used to delegate voting power from one tokenId to another. The tokenId is added to the toTokenId's delegatedTokenIds array which contains all tokenIds that have delegated to the toTokenId. The amount of votes a token has for a checkpoint is determined by its own balance and the balances of all tokens in delegatedTokenIds.

Since delegatedTokenIds is an array, it may contain the same tokenId more than once. This means that the voting power of a tokenId can be multiplied by just adding the same tokenId to the delegatedTokenIds array.

Impact

This issue would allow anyone to increase their voting power without staking more tokens or having legitimate delegations.

A malicous user may also fill the delegatedTokenIds of any tokenId so that when a user wants to delegate to that tokenId the delegate function will revert. This happens when the delegate funtion calls the _writeCheckpoint function which reverts if the delegatedTokenIds array contains 500 or more tokenIds.

Proof Of Concept

The code below shows how a tokenId delegates to a toTokenId in the delegate function. If nCheckpoints > 0, then the tokenId will be added to the toTokenId's delegatedTokenIds array for that checkpoint. The function does not check if the tokenId is currently delegating to the toTokenId or another tokenId which allows a user to repeatedly call the function for the same toTokenId

Affected code:

    if (nCheckpoints > 0) {
            Checkpoint storage checkpoint = checkpoints[toTokenId][nCheckpoints - 1];
            checkpoint.delegatedTokenIds.push(tokenId);
            _writeCheckpoint(toTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
     

After the tokenId is added, _writeCheckpoint is called which will either update the current checkpoint of the toTokenId or create a new checkpoint with the updated delegatedTokenIds array.

The malicious user could use a tokenId with very little voting power to fill up the delegatedTokenIds array so that other tokens are unable to delegate to that toTokenId. They could also delegate to a tokenId they own with the same tokenId and multiply its voting power without actually adding more votes. The getVotes function will then just return voting power of the tokenId * 500 (the delegatedTokenIds array cannot contain more than 500 tokenIds).

Recommended Mitigation Steps

Prevent duplicate tokenIds from being entered in the delegatedTokenIds array when the delegate function is called. This could be done by checking ```delegates[tokenId] == address(0), which if false then remove the tokenId from the delegatedTokenIds array of the toTokenId currently being delegated to.

Lines of code

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

Vulnerability details

The removeDelegation function in VoteEscrowDelegation.sol is used to revoke the delegation given to another tokenId in the delegate function. In other words, removeDelegation is supposed to remove tokenId from the delegatedTokenIds of the toTokenId.

When used in the _transferFrom function, the tokenId being transferred should be delegating to any other tokenIds when transferred to the new owner.

Currently, the removeDelegation function only removes the tokenId given as an argument from the delegatedTokenIds array.

Impact

This issue effectively means that users are unable to revoke delegations.

Proof Of Concept

Affected code:

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

Recommended Mitigation Steps

Consider adding a toTokenId parameter to the removeDelegation function which could be used to find the toTokenId to which the tokenId delegated to. Then the tokenId should be removed from the toTokenId's delegatedTokenIds array. The delegates[tokenId] should return the toTokenId delegate of the tokenId and can be used to remove the delegate from tokenId.

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

1. Should use call instead of transfer Line References GolomTrader.sol#L154

Impact The payable(address).transfer function has a limit of 2300 gas (source). If the receiver has a fallback/receive function that requires more gas then the transaction would revert. If this is the case with the signer of the order, the orders of ordertype 0 might not be fillable.

Recommended Mitigation Steps Consider using the call function instead of transfer to send ETH.

Lines of code

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

Vulnerability details

Description

The fillAsk function in GolomTrader.sol is used to fill a signed order using ETH. The line 217 checks that the amount of ETH given is >= the expected amount that the caller has to pay according to the total amount of the order, the amount of times to fill the order and the amount the caller wants to send as an extra payment.

Impact

Allowing the msg.value to be more than the expected amount to be paid may lead to the user losing the amount that was overspent. Since there does not seem to be any functionality that would allow the operator of the contract to reimburse funds mistakenly sent to the contract, the user may lose the overspent ETH.

Proof Of Concept

Affected code:

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

Recommended Mitigation Steps

A quick fix solution would be to change the >= to == on line 217 which would mean the user would have to provide exactly the amount needed to fill an order. Another solution would be to introduce additional functionality that would allow the operator of the contract to refund any funds sent by mistake to the contract.

1. Should use call instead of transfer

Line References

GolomTrader.sol#L154

Impact

The payable(address).transfer function has a limit of 2300 gas (source). If the receiver has a fallback/receive function that requires more gas then the transaction would revert. If this is the case with the signer of the order, the orders of ordertype 0 might not be fillable.

Recommended Mitigation Steps

Consider using the call function instead of transfer to send ETH.

2. getVotes and getPriorVotes may not return the votes of the tokenId, only its delegators

Line References

VoteEscrowDelegation.sol#L168-L175

VoteEscrowDelegation.sol#L185-L193

Impact

The getVotes function in VoteEscrowDelegation.sol should return the total amount of votes that a tokenId has. Similarly, the getPriorVotes should return the total amount of votes that a tokenId has as of a block number. If no other tokenIds have delegated to the tokenId and it isn't delegated to itself then the getVotes and getPriorVotes functions will return 0 even if the tokenId has a non-zero balance.

The voting power of tokenIds will not be accurate due to the incorrect return values of these functions.

Recommended Mitigation Steps

tokenIds should not be able to delegate to themselves and the getVotes and getPriorVotes functions should set the base votes amount to the balance of the tokenId given as input instead of 0.

3. Floating pragma

Line References

GolomToken.sol

Impact

A floating pragma might allow the contract to be deployed with an outdated compiler which might introduce bugs to the contracts deployed.

Recommended Mitigation Steps

Lock the pragma version to the same version as used by the other contracts in the project.

4. Misleading comments

On line 68 in VoteEscrowDelegation.sol in VoteEscrowDelegation.sol, the comments to do not explain what the delegate function does.

On line 164 in VoteEscrowDelegation.sol in VoteEscrowDelegation.sol, the comment references account which might be misleading if by account it means user since a user may hold multiple tokenIds.

Recommended Mitigation Steps

On line 68, a brief description of what the function does should be added and on line 164 it might be better to replace the reference to account with tokenId.