Golom contest - durianSausage's results

Lines of code

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

Vulnerability details

problem

We should use address(xxx).call{value:xxxx} instead of payable(msg.sender).transfer . Because if msg.sender is a contract, it can use its receive function to reentrance attacking or using some logic vulner abilities to lock the entire contract.

prof:

GolomTrader.sol, 151,

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

mitigation

we should use the following codes.

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            // payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
            address(payAddress).call{value: payAmt}();
        }
    }

tools

vscode

durianSausage Golom audit

Gas optimization

G01: custom error save more gas

problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.

prof

GolomToken.sol', 23, " require(msg.sender == minter, 'GolomToken: only reward distributor can enable'); GolomToken.sol', 42, " require(!isAirdropMinted, 'already minted'); GolomToken.sol', 50, " require(!isGenesisRewardMinted, 'already minted'); GolomToken.sol', 68, " require(minterEnableDate <= block.timestamp, 'GolomToken: wait for timelock'); GolomTrader.sol', 176, " require(signaturesigner == o.signer, 'invalid signature'); GolomTrader.sol', 210, ' require( GolomTrader.sol', 216, " require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm'); GolomTrader.sol', 219, ' require(msg.sender == o.reservedAddress); GolomTrader.sol', 221, " require(o.orderType == 0, 'invalid orderType'); GolomTrader.sol', 225, " require(status == 3, 'order not valid'); GolomTrader.sol', 226, " require(amountRemaining >= amount, 'order already filled'); GolomTrader.sol', 234, " require(amount == 1, 'only 1 erc721 at 1 time'); GolomTrader.sol', 298, " require(amount == 1, 'only 1 erc721 at 1 time'); GolomTrader.sol', 358, " require(amount == 1, 'only 1 erc721 at 1 time'); GolomTrader.sol', 454, " require(distributorEnableDate <= block.timestamp, 'not allowed'); RewardDistributor.sol', 172, " require(address(ve) != address(0), ' VE not added yet'); RewardDistributor.sol', 180, " require(tokenowner == ve.ownerOf(tokenids[tindex]), 'Can only claim for a single Address together'); RewardDistributor.sol', 183, " require(epochs[index] < epoch, 'cant claim for future epochs'); RewardDistributor.sol', 184, " require(claimed[tokenids[tindex]][epochs[index]] == 0, 'cant claim if already claimed'); RewardDistributor.sol', 219, " require(address(ve) != address(0), ' VE not added yet'); RewardDistributor.sol', 291, " require(traderEnableDate <= block.timestamp, 'RewardDistributor: time not over yet'); RewardDistributor.sol', 308, " require(voteEscrowEnableDate <= block.timestamp, 'RewardDistributor: time not over yet'); VoteEscrowCore.sol', 537, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached'); VoteEscrowCore.sol', 893, " require(attachments[_from] == 0 && !voted[_from], 'attached'); VoteEscrowCore.sol', 927, " require(_locked.amount > 0, 'No existing lock found'); VoteEscrowCore.sol', 928, " require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw'); VoteEscrowCore.sol', 944, " require(unlock_time > block.timestamp, 'Can only lock until time in the future'); VoteEscrowCore.sol', 945, " require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max'); VoteEscrowCore.sol', 981, " require(_locked.amount > 0, 'No existing lock found'); VoteEscrowCore.sol', 982, " require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw'); VoteEscrowCore.sol', 995, " require(_locked.end > block.timestamp, 'Lock expired'); VoteEscrowCore.sol', 996, " require(_locked.amount > 0, 'Nothing is locked'); VoteEscrowCore.sol', 997, " require(unlock_time > _locked.end, 'Can only increase lock duration'); VoteEscrowCore.sol', 998, " require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max'); VoteEscrowCore.sol', 1007, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached'); VoteEscrowCore.sol', 1010, ' require(block.timestamp >= _locked.end, "The lock didn't expire"); VoteEscrowCore.sol', 1081, " require(idToOwner[_tokenId] != address(0), 'Query for nonexistent token'); VoteEscrowCore.sol', 1226, " require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved'); VoteEscrowDelegation.sol', 71, " require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed'); VoteEscrowDelegation.sol', 72, " require(this.balanceOfNFT(tokenId) >= MIN_VOTING_POWER_REQUIRED, 'VEDelegation: Need more voting power'); VoteEscrowDelegation.sol', 98, " require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more'); VoteEscrowDelegation.sol', 129, " require(blockNumber < block.number, 'VEDelegation: not yet determined'); VoteEscrowDelegation.sol', 185, " require(blockNumber < block.number, 'VEDelegation: not yet determined'); VoteEscrowDelegation.sol', 210, " require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed'); VoteEscrowDelegation.sol', 238, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

GolomTrader.sol', 151, ' if (payAmt > 0) { GolomTrader.sol', 249, ' if (o.refererrAmt > 0 && referrer != address(0)) { GolomTrader.sol', 386, ' if (o.refererrAmt > 0 && referrer != address(0)) { RewardDistributor.sol', 123, ' if (previousEpochFee > 0) { VoteEscrowCore.sol', 578, ' return size > 0; VoteEscrowCore.sol', 703, ' if (old_locked.end > block.timestamp && old_locked.amount > 0) { VoteEscrowCore.sol', 707, ' if (new_locked.end > block.timestamp && new_locked.amount > 0) { VoteEscrowCore.sol', 726, ' if (_epoch > 0) { VoteEscrowCore.sol', 926, ' require(_value > 0); // dev: need non-zero value VoteEscrowCore.sol', 927, " require(_locked.amount > 0, 'No existing lock found');\n") VoteEscrowCore.sol', 943, ' require(_value > 0); // dev: need non-zero value VoteEscrowCore.sol', 980, ' assert(_value > 0); // dev: need non-zero value VoteEscrowCore.sol', 981, " require(_locked.amount > 0, 'No existing lock found');\n") VoteEscrowCore.sol', 996, " require(_locked.amount > 0, 'Nothing is locked');\n") VoteEscrowDelegation.sol', 77, ' if (nCheckpoints > 0) { VoteEscrowDelegation.sol', 102, ' if (nCheckpoints > 0 && oldCheckpoint.fromBlock == block.number) { VoteEscrowDelegation.sol', 118, ' return nCheckpoints > 0 ? checkpoints[tokenId][nCheckpoints - 1].delegatedTokenIds : myArray;

G03: ARRAY LENTH SHOULD USE MEMoRY VARIABLE LOAD IT

problem

The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP (3 gas), and gets rid of the extra DUP needed to store the stack offset

prof

GolomTrader.sol', 414, ' for (uint256 i = 0; i < proof.length; i++) { RewardDistributor.sol', 142, ' for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol', 156, ' for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol', 179, ' for (uint256 tindex = 0; tindex < tokenids.length; tindex++) { RewardDistributor.sol', 182, ' for (uint256 index = 0; index < epochs.length; index++) { VoteEscrowDelegation.sol', 170, ' for (uint256 index = 0; index < delegated.length; index++) { VoteEscrowDelegation.sol', 188, ' for (uint256 index = 0; index < delegatednft.length; index++) { VoteEscrowDelegation.sol', 198, ' for (uint256 i; i < _array.length; i++) { VoteEscrowDelegation.sol', 200, ' _array[i] = _array[_array.length - 1];

G04: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

GolomTrader.sol', 414, ' for (uint256 i = 0; i < proof.length; i++) { RewardDistributor.sol', 142, ' for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol', 156, ' for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol', 179, ' for (uint256 tindex = 0; tindex < tokenids.length; tindex++) { RewardDistributor.sol', 182, ' for (uint256 index = 0; index < epochs.length; index++) { RewardDistributor.sol', 225, ' for (uint256 index = 0; index < epoch; index++) { RewardDistributor.sol', 257, ' for (uint256 index = 0; index < epoch; index++) { RewardDistributor.sol', 272, ' for (uint256 index = 0; index < epoch; index++) { TokenUriHelper.sol', 137, ' digits++; VoteEscrowDelegation.sol', 170, ' for (uint256 index = 0; index < delegated.length; index++) { VoteEscrowDelegation.sol', 188, ' for (uint256 index = 0; index < delegatednft.length; index++) { VoteEscrowDelegation.sol', 198, ' for (uint256 i; i < _array.length; i++) {

G05: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

VoteEscrowCore.sol', 498, ' ownerToNFTokenCount[_to] += 1; VoteEscrowCore.sol', 511, ' ownerToNFTokenCount[_from] -= 1;

G06: SPLITTING REQUIRE() STATEMENTS THAT USE && SAVES GAS

prof

VoteEscrowCore.sol', 537, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached'); VoteEscrowCore.sol', 893, " require(attachments[_from] == 0 && !voted[_from], 'attached'); VoteEscrowCore.sol', 1007, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached'); VoteEscrowDelegation.sol', 238, " require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

G07: the assert() function when false, uses up all the remaining gas and reverts all the changes made.

problem

assert(false) compiles to 0xfe, which is an invalid opcode, using up all remaining gas, and reverting all changes. require(false) compiles to 0xfd which is the REVERT opcode, meaning it will refund the remaining gas. The opcode can also return a value (useful for debugging), but I don't believe that is supported in Solidity as of this moment. (2017-11-21)

prof

VoteEscrowCore.sol', 492, ' assert(idToOwner[_tokenId] == address(0)); VoteEscrowCore.sol', 505, ' assert(idToOwner[_tokenId] == _from); VoteEscrowCore.sol', 518, ' assert(idToOwner[_tokenId] == _owner); VoteEscrowCore.sol', 665, ' assert(_operator != msg.sender); VoteEscrowCore.sol', 678, ' assert(_to != address(0)); VoteEscrowCore.sol', 860, ' assert(IERC20(token).transferFrom(from, address(this), _value)); VoteEscrowCore.sol', 976, ' assert(_isApprovedOrOwner(msg.sender, _tokenId)); VoteEscrowCore.sol', 980, ' assert(_value > 0); // dev: need non-zero value VoteEscrowCore.sol', 990, ' assert(_isApprovedOrOwner(msg.sender, _tokenId)); VoteEscrowCore.sol', 1006, ' assert(_isApprovedOrOwner(msg.sender, _tokenId)); VoteEscrowCore.sol', 1022, ' assert(IERC20(token).transfer(msg.sender, value)); VoteEscrowCore.sol', 1109, ' assert(_block <= block.number); VoteEscrowCore.sol', 1205, ' assert(_block <= block.number);

G08: STATE VARIABLES CAN BE PACKED INTO FEWER STORAGE SLOTS

---

GolomToken.sol, 15-21

    address public minter;

    uint256 public minterEnableDate;
    address public pendingMinter;

    bool public isAirdropMinted;
    bool public isGenesisRewardMinted;

we should reorder the define sequence.

    uint256 public minterEnableDate;
    bool public isAirdropMinted;
	address public pendingMinter;
    bool public isGenesisRewardMinted;
	 address public minter;

---

VoteEscrowCore.sol, 320 and 347-358 should put together.

    bytes4 internal constant ERC165_INTERFACE_ID = 0x01ffc9a7;

    /// @dev ERC165 interface ID of ERC721
    bytes4 internal constant ERC721_INTERFACE_ID = 0x80ac58cd;

    /// @dev ERC165 interface ID of ERC721Metadata
    bytes4 internal constant ERC721_METADATA_INTERFACE_ID = 0x5b5e139f;

    /// @dev reentrancy guard
	uint8 public constant decimals = 18;
    uint8 internal constant _not_entered = 1;
    uint8 internal constant _entered = 2;
    uint8 internal _entered_state = 1;

G09: USING BOOLS FOR STORAGE INCURS OVERHEAD

problem

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

prof

VoteEscrowCore.sol, 314, mapping(uint256 => bool) public voted; VoteEscrowCore.sol, 341, mapping(address => mapping(address => bool)) internal ownerToOperators; VoteEscrowCore.sol, 344, mapping(bytes4 => bool) internal supportedInterfaces;

G10: USAGE OF UINTS/INTS SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

problem

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

prof

VoteEscrowCore.sol, 697

G11: resign the default value to the variables.

problem

resign the default value to the variables will cost more gas.

prof

'GolomTrader.sol', 414, ' for (uint256 i = 0; i < proof.length; i++) { 'RewardDistributor.sol', 44, ' uint256 public epoch = 0; 'RewardDistributor.sol', 141, ' uint256 reward = 0; 'RewardDistributor.sol', 142, ' for (uint256 index = 0; index < epochs.length; index++) { 'RewardDistributor.sol', 155, ' uint256 reward = 0; 'RewardDistributor.sol', 156, ' for (uint256 index = 0; index < epochs.length; index++) { 'RewardDistributor.sol', 174, ' uint256 reward = 0; 'RewardDistributor.sol', 175, ' uint256 rewardEth = 0; 'RewardDistributor.sol', 179, ' for (uint256 tindex = 0; tindex < tokenids.length; tindex++) { 'RewardDistributor.sol', 182, ' for (uint256 index = 0; index < epochs.length; index++) { 'RewardDistributor.sol', 221, ' uint256 reward = 0; 'RewardDistributor.sol', 222, ' uint256 rewardEth = 0; 'RewardDistributor.sol', 225, ' for (uint256 index = 0; index < epoch; index++) { 'RewardDistributor.sol', 256, ' uint256 reward = 0; 'RewardDistributor.sol', 257, ' for (uint256 index = 0; index < epoch; index++) { 'RewardDistributor.sol', 271, ' uint256 reward = 0; 'RewardDistributor.sol', 272, ' for (uint256 index = 0; index < epoch; index++) { 'TokenUriHelper.sol', 28, ' let i := 0 'VoteEscrowCore.sol', 734, ' uint256 block_slope = 0; // dblock/dt 'VoteEscrowCore.sol', 744, ' for (uint256 i = 0; i < 255; ++i) { 'VoteEscrowCore.sol', 1041, ' uint256 _min = 0; 'VoteEscrowCore.sol', 1043, ' for (uint256 i = 0; i < 128; ++i) { 'VoteEscrowCore.sol', 1112, ' uint256 _min = 0; 'VoteEscrowCore.sol', 1114, ' for (uint256 i = 0; i < 128; ++i) { 'VoteEscrowCore.sol', 1132, ' uint256 d_block = 0; 'VoteEscrowCore.sol', 1133, ' uint256 d_t = 0; 'VoteEscrowCore.sol', 1166, ' for (uint256 i = 0; i < 255; ++i) { 'VoteEscrowCore.sol', 1168, ' int128 d_slope = 0; 'VoteEscrowCore.sol', 1183, ' last_point.bias = 0; 'VoteEscrowCore.sol', 1210, ' uint256 dt = 0; 'VoteEscrowDelegation.sol', 49, ' uint256 public MIN_VOTING_POWER_REQUIRED = 0; 'VoteEscrowDelegation.sol', 146, ' uint256 lower = 0; 'VoteEscrowDelegation.sol', 169, ' uint256 votes = 0; 'VoteEscrowDelegation.sol', 170, ' for (uint256 index = 0; index < delegated.length; index++) { 'VoteEscrowDelegation.sol', 187, ' uint256 votes = 0; 'VoteEscrowDelegation.sol', 188, ' for (uint256 index = 0; index < delegatednft.length; index++) {