Golom contest - async's results

Lines of code

https://github.com/golom-protocol/contracts/blob/4e84d5c2115d163ca54a1729da46164e8cf4df6d/contracts/vote-escrow/VoteEscrowDelegation.sol#L101-L108

Vulnerability details

Impact

In VoteEscrowDelegation._writeCheckpoint, when the checkpoint is overwritten in the same block the new value is set with memory oldCheckpoint and thus is never written to storage.

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

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

oldCheckpoint.delegatedTokenIds = _delegatedTokenIds; 
}

Users that remove and delegate a token (or call delegate on the same token twice) in the same block will only have their first delegation persisted.

Proof of Concept

• User delegates a tokenId by calling delegate.
• In the same block, the user decides to delgate the same token to a different token ID and calls delegate again which calls _writeCheckpoint. Since this is the second transaction in the same block the if statement in the code block above executes and stores _delegatedTokenIds in memory oldCheckpoint, thus not persisting the array of _delegatedTokenIds in the checkpoint.

Tools Used

Manual analysis

Recommended Mitigation Steps

Define the oldCheckpoint variable as a storage pointer:

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

Return statement will never execute

In GolomTrader L178

• The return (0,...) will never execute because the preceeding require statement prevents it.

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

Other protocols that wish to integrate with Golom will expect a return of 0 if the orderStatus is invalid. The natSpec comment for validateOrder states that "OrderStatus = 0 , if signature is invalid". If a protocol wishes to extend or integrate with Golom there may be unintended consequences if they expect a return value of 0 from an invalid signature.

Signature malleability in validateOrder

In GolomTrader L176

• The ecrecover function is used to verify the signaturesigner. Ecrecover is known to be susceptible to signature malleabiliity which could lead to replay attacks. Since the message hash includes a fresh nonce this is not much of a concern. However, consider using OpenZeppelin ECDSA.recover to perform additional checks.

Require statements do not return error messages

In GolomTrader

• Several require statments return no error message (L220, L285, L293 etc...). It is good practice to elaborate on why a transaction reverted so users and developers can react to that information.

fillBid comparison operator bug

In GolomTrader L286

• The comparison operator should be >=

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

OrderCancelled event can be spammed

In GolomTrader

• In cancelOrder there is no check to validate if an order has been previously canceled. This would allow an individual to spam the OrderCancelled event and increment the filled[hashStruct] unbounded which may have unintended consequences.

_settleBalances inaccurate natSpec comment

In GolomTrader L370

• The natSpec for _settleBalances states that "@param o the Order struct to be filled must be orderType 1". This seems inaccurate because _settleBalances is called by both fillBid (order type 1) and fillCriteriaBid (order type 2).

Add events for important protocol changes

In GolomTrader L444

• Add events for setDistributor and executeSetDistributor
• Events should be emitted when signifcant changes happen in a contract. There are time-delays for changing the distributor but no event is emitted to notify downstreams consumers that the distributor is being changed. Consider adding events to important setter functions to notify users of pending protocol changes.

In RewardDistributor L285

• Add events for changeTrader, executeChangeTrader, addVoteEscrow and executeAddVoteEscrow

Unused variables

In RewardDistributor L57

• There are unused mappings which can be removed for gas savings.
• rewardLP & claimedUpto

Variable shadowing

In VoteEscrowDelegation

• There are several variables which shadow other definitions in VoteEscrowCore. This can lead to security issues if this condition goes unnoticed. See the swcregistry description.

Gas savings

In GolomTrader

The Order struct can be packed more efficently to reduce storage costs and save gas. Each storage slot in EVM holds a total of 32 bytes so if we reduce the storage size of some variables we can pack the storage slot and save gas on reads from storage.

• uint8 orderType - this can be changed to a smaller uint since there are only 3 order types. A uint8 is 1 byte
• uint32 nonce - a uint32 has a max value of 4,294,967,295 - 4 bytes, it's doubtful a nonce will ever reach this number
• uint48 deadline - a uint48 has a max value of 241,474,976,710,656 - 6bytes, it's doubtful a deadline will surpass this number

struct Order { //@audit can pack this more efficently

address collection; // NFT contract address 20 bytes

uint256 tokenId; // order for which tokenId of the collection 32 bytes

// @audit the 4 variables below add up to 32 bytes and will pack in to a single storage slot. The ordering of variables is important when trying to efficiently pack slots which is why I restructured their declaration ordering.

address signer; // maker of order address 20 bytes

+ uint8 orderType; // 1 byte

+ uint32 nonce; // nonce of order usefull for cancelling in bulk //4 bytes

+ uint48 deadline; // timestamp till order is valid epoch timestamp in secs //@audit change to uint48 - max value 241,474,976,710,656 6 bytes

+ bool isERC721; // standard of the collection , if 721 then true , if 1155 then false //@audit pack with orderType

- uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root //@audit change to uint8

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 //@audit pack with orderType

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 //@audit uint32

- uint256 deadline; // timestamp till order is valid epoch timestamp in secs //@audit change to uint48 - max value 241,474,976,710,656 6 bytes



uint8 v;

bytes32 r;

bytes32 s;

}

In RewardDistributor

• startTime can be set as a constant variable.
• L99 - stale comment can be removed
• L100 Consider declaring a new variable MAX_SUPPLY = 1000000000 * 10**18 so it's easier to reason about.
• L112

uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /

rewardToken.totalSupply();

uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

Consider storing rewardToken.totalSupply & rewardToken.balanceOf(address(ve))) as local variables. This will save gas on two external calls.

In VoteEscrowDelegation

• L147 There's no need to initialize lower with a 0 value and leaving it uninitialized will save gas.