Golom contest - carlitox477's results

Lines of code

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

Vulnerability details

Impact

Given it is called by _transferFrom function and removeDelegation is external, this function would never be call and _transferFrom will always revert, contract deployment woul be a waste of time

Mitagation

Just replace line 210 for ````function removeDelegation(uint256 tokenId) public``

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

01 - payEther use transfer instead of call Replace line 154 for

payable(payAddress).call{value: payAmt}("") Reason

Non-Critical

GolemTrader.sol

01 - Change ER1155 name to IERC1155_Golem (code style)

Interface ERC1155 confuse code reader with openzeppelin contracts, change it to IERC1155_Golem

02 - Add safeTransferFrom to ERC1155 interface and use it

Using safeTransferFrom is a better practise than using transferFrom. Reasons

03 - Change ERC20 to IWETH_Golem

Interface ERC20 confuse code reader with openzeppelin contracts, change it to IERCWETH_Golem

04 - Change variable names to openzeppelin standards

In interface ERC20, change it variable names to openzeppelin standards to facilitate understandment for devs.

• src to from
• dst to to
• wad to amount

05 - Testing GolemTrader mistake: wrong golomToken initialization

In GolemTrader.spect.ts replace line 95 for golomToken = (await (await GolomTokenArtifacts).deploy(await governance.getAddress())) as GolomTokenTypes;

To set the correct governance, otherwise the test will not work correctly

06 - Testing mistaken initialization

In GolemTrader.spect.ts, in test for function #fillAsk add a before each statement to set the rewardDistributor as minter, otherwise some will fail

Take into account 05 - Testing GolemTrader mistake: wrong golomToken initialization and add next lines at test start for function #fillAsk:

beforeEach(async()=>{
    await golomToken.connect(governance).mintGenesisReward(rewardDistributor.address)
    await golomToken.connect(governance).setMinter(rewardDistributor.address)
    await golomToken.connect(governance).executeSetMinter()
})

07 - Hardcoded WETH

It is usually recommended not to hardcode variables. Send the address a parameter and initialize WETH in the constructor

constructor(address _governance,address _weth) {
        WETH = ERC20(_weth);
        //The other code keeps the same
        /*
            ...
        /*
    }

08 - hashPayment function does not follow naming convention

Change it name to _hashPayment to respect private/internal function naming convention

09 - payEther function does not follow naming convention

Change it name to _payEther to respect private/internal function naming convention

10 - We can change distributor fee

The protocol can benefit from setting the distributor fee to zero in for it promotion, or increase it if the project is super succesful. However the contract fix this fee so this can not happen.

Idea:

1. Create a storage public uint256 variable and callIt distributorFeeBasisPoints
2. Introduce logic in the contract to initialize it in the constructor
3. In fillAsk, after transfering the token create a variable _distributorFeeBasisPoints (this to save gas)
4. Replace all 50 ocurrancies in fillAsk for the variable _distributorFeeBasisPoints
5. Create a function to modify distributorFeeBasisPoints. Maybe add a require to set a max possible fee.

11 - Change variables names in fillAsk and fillBid function

For better code understandment, change next variable names

• o to order
• p to extraPayment

12 - fillBid is not called by anyone other contract function

Change it visibility to external

Reward distributor

01 - secsInDay does not respect constant naming convention

Should change it name to SECS_IN_DAY to respect constant variable naming convention

02 - ERC20 interface naming

Change it name to IERC20_Golem just for not confusing with openzeppelin contracts

Change variable names to openzeppelin standards to better understandment for new devs

• src to from
• dst to to
• wad to amount

03 - Send WETH address by parameter

This will be better during testing. Consider making startTime storage value a constant or send it by parameter (this second options seems better)

04 - Change addFee visibility to external

This function isn't called by any other function in the contract.

05 - Constructor doesn't allow to initialize startTime

It initialization is hardcoded, it is a better practise to send it value by parameter. New suggested constructor:

constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance,
        uint256 _startTime
    ) {
        require(_startTime>=block.timestamp)
        startTime = _startTime;
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        
    }

VoteEscrow

01 - Change MIN_VOTING_POWER_REQUIRED notation

Change it to minVotingPowerRequired notation to lower cammel to follow variable naming convention

02 - constructor does not initialize MIN_VOTING_POWER_REQUIRED

It will be set by default to zero. Is it meant to be like this?

03 - Storage variable shadowing in different methods

Although this does not seem to affect the code, it is a bad practise. Should change variable names for better code style:

• In function delegate change tokenId for _tokenId and checkpoint for _checkpoint
• In function _getCurrentDelegated change tokenId for _tokenId
• In function getVotes change tokenId for _tokenId
• In function getPriorVotes change tokenId for _tokenId
• In function removeDelegation change tokenId for _tokenId and checkpoint for _checkpoint

Extra - You have gave out your alchemy - api key

This code has a hardcoded commented api key in hardhat.config.ts

I suggest to erase it from and start putting your key in a .env file

---

Low

GolemTrader.sol

validateOrder should return status 0 for an order with a wrong signature or change it visibility to internal

Giving that its visibility is public view function, this function can be queried by anyone. However, when it is given a wrongly signed order, the function is reverted instead of return a 0 status

Prof of concept

In GolemTrader.specs.ts add:

describe('#validateOrder', ()=>{
    it('Should return status 0 for an order with a wrong signature',async ()=>{
        //Current status: the transaction is reverted. Solution: try catch code
        //Correcting this function implies correcting cancelOrder by addin a require(status!=0) after calling validateOrder

        let exchangeAmount = ethers.utils.parseEther('1'); // cut for the exchanges
        let prePaymentAmt = ethers.utils.parseEther('0.25'); // royalty cut
        let totalAmt = ethers.utils.parseEther('1');
        let tokenId = await testErc721.current();
        let wrongSigner = governance as any

        const order = {
            collection: testErc721.address,
            tokenId: tokenId,
            signer: await maker.getAddress(),
            orderType: 0,
            totalAmt: totalAmt,
            exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
            prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
            isERC721: true,
            tokenAmt: 1,
            refererrAmt: 0,
            root: '0x0000000000000000000000000000000000000000000000000000000000000000',
            reservedAddress: constants.AddressZero,
            nonce: 0,
            deadline: Date.now() + 100000,
            r: '',
            s: '',
            v: 0,
        };


        let signature = (await wrongSigner._signTypedData(domain, types, order)).substring(2);
        //let signature = (await maker._signTypedData(domain, types, order)).substring(2);

        order.r = '0x' + signature.substring(0, 64);
        order.s = '0x' + signature.substring(64, 128);
        order.v = parseInt(signature.substring(128, 130), 16);

        await expect(golomTrader.validateOrder(order), 'Function reverted').to.not.be.reverted
        const result=await golomTrader.validateOrder(order)
        expect((result[0] as BigNumber).eq(0),"Return value different from zero").to.be.true
    })
})

Mitigation

1. Remove require(signaturesigner == o.signer, 'invalid signature'); from validateOrder function
2. In function cancelOrder remove line (, bytes32 hashStruct, ) = validateOrder(o); and replace it for

(uint256 status, bytes32 hashStruct, ) = validateOrder(o);
require(status != 0, 'invalid signature');

Is is important to point out that this issue can scale to medium or even high if there are off-chain dependant function or external contract which functionality relays on this function

VoteEscrow

RemoveDelegation should emmit at least one event

DelegateChanged or/and DelegateVotesChanged should be emitted given the semantics. The lack of emission may affect off chain applications. For instance, a dependant subgraph

GolomTrader.sol

01 - payEther use transfer instead of call

Replace line 154 for

payable(payAddress).call{value: payAmt}("")

Reason

02 - Repetead operation in fillAsk

After transfering the token and before doing the payment add uint256 distributorFee=(o.totalAmt * 50) / 10000. Then replace every (o.totalAmt * 50) / 10000 occurancy by this new variable to save gas. Previous gas spent 390095/393890/393346, now 38977/393572/393025

RewardDistributor

01 - epoch storage value should be removed

It default value will be zero

02 - Create a constant variable called MAX_SUPPLY

Create the constant MAX_SUPPLY = 1000000000 * 10**18 to save gas at deployment and in function addFee

03 - In addFee do a single rewardToken.totalSupply() call

At function start, add uint256 _rewardToken= rewardToken.totalSupply() to do just one call to the external contract, and change every other occurancy of rewardToken.totalSupply() in this function for this new variable (this saves gas)

04 - In add fee replace rewardToken.balanceOf(address(ve))

At the begining of second if statmente add a declaration uint256 _rewarTokenBalanceOfVe = rewardToken.balanceOf(address(ve)) and replace every other occurancy of rewardToken.balanceOf(address(ve)) for this new variable to avoid accessing to external contract (sooner or later to storage) 2 times and save gas

05 - In addFee create a variable to asignate tokenToEmit - stakerReward value

This avoid an operation making to math operations after line uint256 previousEpochFee = epochTotalFee[epoch]; replace

rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

For:

totalRewards = tokenToEmit - stakerReward;
rewardTrader[epoch] = (totalRewards * 67) / 100;
rewardExchange[epoch] = (totalRewards * 33) / 100;

RewardDistributor

01 - Remove reward variable initialization in traderClaim

It default value is zero, there is no need to spend gas in initializing it

02 - Remove reward variable initialization in exchangeClaim

It default value is zero, there is no need to spend gas in initializing it

03 - In addFee put epoch storage variable in memory variable

At second if statment start put epoch storage variable in a memory variable called uint256 _epoch = epoch Consider that doing this also implies change this line for:

_epoch = _epoch + 1
epoch = _epoch

04 - In exchangeClaim, traderClaim, multiStakerClaim,stakerRewards, traderRewards, exchangeRewards put epoch storage variable in a memory variable

Just add an _epoch initialization uint256 _epoch = epoch and replace epoch occurancie in the loop, reducing multiple storage access saving gas.

05 - multiStakerClaim,stakerRewards, stakerRewards: eliminate reward and rewardEth initialization

Their defaults values are already zero.

06 - traderClaim, exchangeClaim, traderRewards,exchangeRewards: eliminate reward initialization

Their defaults values are already zero.

VoteEscrowDelegation

01 - DelegateVotesChanged is never emmited

This could be a serious issue if off chain aps depends on it. However, if it isn't needed it can be eliminated and save gas at deployment