Golom contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

When numCheckpoints[toTokenId] is 0, delegate calls _writeCheckpoint(toTokenId, 0, [tokenId]). However, _writeCheckpoint performs a nCheckpoints - 1 calculation that will underflow in this case (when nCheckpoints is 0).

Recommended Mitigation Steps

Handle the case nCheckpoints == 0 separately in _writeCheckpoint.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L391 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L397

Vulnerability details

Impact

In _settleBalances, the variable protocolfee is already multiplied by the amount. However, in line 391 or 397 (depending on if a refferer is used), it is multiplied by amount again. Because of that, too little ETH is transferred to the seller and the difference is stuck forever in the contract.

Proof Of Concept

Let's consider the simple example with o.totalAmt = 10_000 and amount = 100. We have protocolFee = ((10_000 * 50) / 10000) * 100 = 5_000. Let's say (for the sake of simplicity) that there is no referrer and that o.exchange.paymentAmt = 0, o.prePayment.paymentAmt = 0, p.paymentAmt = 0. Then, we have:

payEther((10_000 - 5_000) * 100, msg.sender) = payEther(500_000, msg.sender))

Therefore, to summarize:

• o.totalAmt * amount = 1_000_000 was transferred from the bidder (o.signer) to GolomTrader.
• protocolFee = 5_000 was transferred from GolomTrader to the distributor.
• 500,000 was transferred to msg.sender, i.e. the seller.

The remaining 495,500 are stuck in GolomTrader.

Recommended Mitigation Steps

Set the maximumSupply to 1 billion on the token itself such that the user can be sure that no more tokens will ever exist.

Lines of code

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

Vulnerability details

Impact

In delegate, when a user delegates to a new tokenId, the tokenId is not removed from the current delegatee. Therefore, one user can easily multiply his voting power, which makes the toking useless for voting / governance decisions.

Proof Of Concept

Bob owns the token with ID 1 with a current balance of 1000. He also owns tokens 2, 3, 4, 5. Therefore, he calls delegate(1, 2), delegate(1, 3), delegate(1, 4), delegate(1, 5). Now, if there is a governance decision and getVotes is called, Bobs balance of 1000 is included in token 2, 3, 4, and 5. Therefore, he quadrupled the voting power of token 1.

Recommended Mitigation Steps

Remove the entry in delegatedTokenIds of the old delegatee or simply call removeDelegation first.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L100 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L384

Vulnerability details

Impact

When the reward token supply is larger than a billion, epochTotalFee is no longer updated. However, the fee is still transferred to the reward distributor. But because epochTotalFee is no longer updated for those epochs, calling multiStakerClaim with them will not result in any ETH. Therefore, those tokens are forever stuck in the RewardDistributor.

Recommended Mitigation Steps

Still keep track of the accumulated fees, even if no new tokens are minted anymore.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L154

Vulnerability details

Impact

The payEther function uses transfer which has only a 2300 gas stipend for transferring ETH. Depending on the logic of the retriever (e.g., a smart contract that performs some storage read and writes on receive, for instance a multi-sig), this may not be sufficient and the transfer can revert. See also https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ for a discussion of this issue.

Recommended Mitigation Steps

Use call instead, i.e.:

(bool success, ) = payable(payAddress).call{amount}("");
require(success, "Transfer failed.");

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L301 https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L361

Vulnerability details

Impact

The implementation uses transferFrom instead of safeTranferFrom for transferring ERC721 tokens. This is discouraged according to the OpenZeppelin documentation. Furthermore, there are certain contracts (e.g., https://github.com/sz-piotr/eth-card-game/blob/master/src/ethereum/contracts/ERC721Market.sol#L20-L31) that have a logic in their onERC721Received function, which will not be triggered.

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L604

Vulnerability details

Impact

In the safeTransferFrom of VoteEscrowCore, the ERC721 check is wrong. It is only checked if the the contract reverts on the onERC721Received call (or does not implement the function), but not if the return value is equal to IERC721Receiver.onERC721Received.selector, which is the requirement of EIP-721. Therefore, every other protocol that uses these tokens might not work correctly or the token might introduce vulnerabilities there.

Recommended Mitigation Steps

Check that retval == IERC721Receiver.onERC721Received.selector, see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L402 for a correct implementation.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L135

Vulnerability details

Impact

If a user pays too much ETH when filling an ask, it is not reimbursed too him. Moreover, it is also not transferred to someone else (e.g., the seller), meaning it is stuck in GolomTrader and therefore lost forever.

Recommended Mitigation Steps

Consider reimbursing the user when he pays too much ETH.

• For traderRewards, the comment about the addr param (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L253) is wrong (it is not the NFT ID)
• I think it would make sens to also return the epochs with non-zero rewards for traderRewards and exchangeRewards in RewardDistributor (like it is done with the nclaimed epochs in stakerRewards). For traderClaim and exchangeClaim, the array of epochs also needs to be provided. However, there is currently no (automated) way to retrieve them, so it will for instance be also hard to write a smart contract / protocol that automatically retrieves them.
• An exchange might want to transfer the tokens to a different address (as the exchange address might not be able to retrieve the tokens), but this functionality is not provided in exchangeClaim.
• The return value (boolean) of rewardToken.transfer (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L151) is ignored. While this is not a problem witht he current token implementation (that reverts on failure), it is not ERC20 compatible, meaning the system will not work correctly with some ERC20 tokens.
• You might want to check for the zero address in the GolomToken constructor (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L29) to avoid having an unusable deployment.
• The docs of executeSetMinter() state that "If being called first time, there won't be any timelock" (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/governance/GolomToken.sol#L64). This is not true, the owner can also set the minter afterwards to the zero address (via setMinter), which allows him to then (after he waited 24 hours to set it to 0) change it immediately to any address (as the current one is the zero address).
• Since Solidity 0.8.0, block.chainid could be used instead of inline assembly (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L98)
• There is a hardhat/console.sol import in RewardDistributor (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L9)
• TokenUriHelper returns the wrong mimetype (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/TokenUriHelper.sol#L125). data:application/json;base64, would be used when the whole JSON payload is base64 encoded, but only the image, i.e. one element of the payload, is (and this is already correctly annotated with base64).
• In VoteEscrowCore, assert is used in many places. It would be good in my opinion to replace them with require, as the gas is refunded then (for instance, there is no reason to not refund the gas when a user accidentally calls setApprovalForAll and passes its own address as _operator).
• In VoteEscrowCore, when merge is called, supply is still increased in _deposit_for (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L843), although no actual increase of supply happens. Therefore, the value of supply is wrong as soon as a merge operation was executed.
• There is the potential for overflows in VoteEscrowCore (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L847, https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L901, https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/vote-escrow/VoteEscrowCore.sol#L1012), as SafeCast is not used. However, an exploit is unlikely in practice, as the attacker would need to have this many tokens.
• Because ECDSA signatures are malleable, it is considered best practice to check that v is equal to 27 or 28 and s not too high (see e.g. here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L57), which is not done (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L176).
• validateOrder does not work as specified. According to the specification, 0 is returned if the signature is invalid. However, 0 can never be returned. If signaturesigner != o.signer, the function reverts (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L177), so line 179 is dead code and the specification wrong.
• According to the documentation, referrer is the "amt to pay to the address that helps in filling your order". In this case, this parameter does not really make sense for fillBid. Bids are always filled by the taker itself (as the NFTs are transferred from them to the signer), so there is no scenario where they are filled by a different user.
• The WETH transferFrom ignore value is ignored (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L382), which is not ERC-20 compliant. While this is not too problematic for the current implementation (that reverts on failure), another WETH implementation might return false on failure.
• The comment for delegate states "Explain to an end user what this does" (https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L68)
• delegate can only be called by the owner of a token, not by someone that is approved (https://github.com/code-423n4/2022-07-golom/blob/8f198624b97addbbe9602a451c908ea51bd3357c/contracts/vote-escrow/VoteEscrowDelegation.sol#L72). This can be very problematic for contracts that want to build on top of the tokens, as they cannot delegate the votes for the users (even if he has approved the tokens). An alternative to operators would be to implement delegateBySig like OpenZeppelin does in their ERC20Votes implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Votes.sol#L133
• For ERC1155, a seller might want to have "all-or-nothing" offers that cannot be partially filled (maybe with a rebate for buying all tokens). This is currently not supported, a user can always partially fill.
• In fillCriteriaBid, the paymentAmt and the protocol fee are not included in the calculation of the required totalAmt (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L342). While this is not directly exploitable (as _settleBalances subtracts these values from totalAmt), it can be confusing and the check is wrong (and different from fillBid).
• In fillBid, the protocol fee is not included in the calculation of the required totalAmt (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L287)
• The comment for stakerRewards is wrong (https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/rewards/RewardDistributor.sol#L214). It is mentioned that "all ids in the list must belong to 1 address", but it is not a list, simply one token ID.
• Because of rounding errors when calculating rewardTrader and rewardExchange, the sum of rewardStaker, rewardTrader and rewardExchange will not always be equal to tokenToEmit. Consider doing a subtraction for rewardExchange (i.e., assigning all remaining tokens to rewardExchange) instead using a multiplication & division for both rewardTrader and rewardExchange.