Golom contest - 0x4non's results

Lines of code

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

Vulnerability details

Impact

Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes (such as with EIP-2929 in the recent Berlin fork) may break deployed contracts.

There are three usages of transfer() which may fail if the destination is a contract which uses repriced opcodes.

See reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual revision

Recommended Mitigation Steps

Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

function payEther(uint256 payAmt, address payAddress) internal {
    if (payAmt > 0) {
        // if royalty has to be paid
        (bool result, ) = payAddress.call{value: payAmt}(""); // royalty transfer to royaltyaddress
        require(result, "Failed to send Ether");
    }
}

Lines of code

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

Vulnerability details

Impact

if you use transferFrom to transfer ERC721, the receiver would not run the expected hooks thath should run.

Proof of Concept

Send a ERC-721 compatible token to an contract receiver using transferFrom, the contract will not run hooks expected. Take in mind that youy could send the NFT to a unprepared contract with transferFromand the NFT will be locked forever, but if you do with safeTransferFromif the contract is not got the expected hooks it will revert.

Tools Used

Manual revision

Recommended Mitigation Steps

Take in mind change transferFrom to safeTransfeFrom

Lines of code

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

Vulnerability details

Impact

The callback from the onERC721Received is never checked

Proof of Concept

If you send a ERC721 using safeTransferFrom and the onERC721Received returns falsethe transaction will not be reverted.

Checke the EIP 721:

function onERC721Received: "Return of other than the magic value MUST result in the transaction being reverted."

Tools Used

Manual revision

Recommended Mitigation Steps

Take a look to the OZ implementation and check for retval; https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.7.2/contracts/token/ERC721/ERC721.sol#L401-L402

LOW

Open pragma on GolomToken.sol

Contracts should be deployed with the same compiler version/flags where they have been tested with. Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version that might introduce bugs that affect the contract system negatively.

Consider change pragma solidity ^0.8.11; to pragma solidity 0.8.11;

Unused import hardhat/console.sol

Consider remove this import; import 'hardhat/console.sol'; https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L9

Shadow variables

Consider renaming shadow variables to avoid mistakes; In VoteEscrowDelegation.sol you are shadowing varaibles tokenId and checkpoint. There are two thing you could do, you can set this as private on VoteEscrowCore.sol or you could renames this variables in VoteEscrowDelegation.sol

Critical function dont emit evets

Criticial functions changeTrader, executeChangeTrader, addVoteEscrow, executeAddVoteEscrow of RewardDistributor.sol should emit events.

Criticial function changeMinVotingPower from VoteEscrowDelegation.sol should emit events.

QA

Use the format import {Contract} from "./contract.sol";

Use the named import formar to control contract imports; https://docs.soliditylang.org/en/v0.8.13/layout-of-source-files.html?highlight=import%20%7B#syntax-and-semantics

Remove commented code

If you are not using this code consider removing it; https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/GolomAirdrop.sol#L134-L140

Separate interfaces in a new file

Consider separate interfaces from contract and moving interfaces into new files; https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/GolomAirdrop.sol#L12-L48

Consider separate interfaces from contract and moving interfaces into new files; https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L11-L42