Golom contest - rajatbeladiya's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298-L311

Vulnerability details

Impact

not able to set voteEscrow contract address to RewardDistributor. So stakerRewards() and multiStakerClaim() will not work and no one will be able to claim their rewards.

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L172 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L215

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L299-L301

pendingVoteEscrow used to initialize ve but pendingVoteEscrow will be zero address.

Tools Used

Manual Analysis

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L300 use _voteEscrow insted of pendingVoteEscrow

Lines of code

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

Vulnerability details

Impact

numCheckPoints is used to calculate checkpoints for each delegated tokenId

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

here, initial nCheckpoints is set to zero, substract it to by 1 will throw Underflow error. So delegate will not work

Proof of Concept

    it.only('should delegate', async () => {
        let ve_underlying_amount = ethers.utils.parseEther('10');
        let ve_underlying_approve_amount = ethers.utils.parseUnits('10', 28);

        await golomToken.mint(maker.address, ve_underlying_approve_amount);
        await golomToken.approve(voteEscrow.address, ve_underlying_approve_amount);

        const lockDuration = 365 * 24 * 3600 * 4; // 4  year
        const rewards_starts_at = 2659187519;
        await ethers.provider.send('evm_setNextBlockTimestamp', [rewards_starts_at]);

        let re = await voteEscrow.create_lock(ve_underlying_amount, lockDuration);

        await voteEscrow.delegate(1, 1); // error with underflow error
    });

    `Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)`

Tools Used

Manual Analysis

Recommended Mitigation Steps

add below check to _writeCheckpoint

    Checkpoint memory oldCheckpoint;
    if (nCheckpoints > 0) {
        oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
    }

Lines of code

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

Vulnerability details

Impact

delegate call not checking for existing Ids and pushes same tokenId multiple times which is also used for calculate votes. Votes can play a key role in governance ( did not checked governance contract, as it is not in the scope ), bad actor can manipulate proposals in any way.

Proof of Concept

    it.only('should increase vote on same delegate call', async () => {
        try {

            let ve_underlying_amount = ethers.utils.parseEther('10');
            let ve_underlying_approve_amount = ethers.utils.parseUnits('10', 28);

            await golomToken.mint(maker.address, ve_underlying_approve_amount);
            await golomToken.approve(voteEscrow.address, ve_underlying_approve_amount);

            const lockDuration = 365 * 24 * 3600 * 4; // 4  year
            const rewards_starts_at = 2659187519;
            await ethers.provider.send('evm_setNextBlockTimestamp', [rewards_starts_at]);

            let re = await voteEscrow.create_lock(ve_underlying_amount, lockDuration);

            await voteEscrow.delegate(1, 1);
            const votes1 = await voteEscrow.getVotes(1);
            await voteEscrow.delegate(1, 1);
            const votes2 = await voteEscrow.getVotes(1);
            await voteEscrow.delegate(1, 1);
            const votes3 = await voteEscrow.getVotes(1);
            
            console.log('votes1===', Number(votes1)); // 9981963390993345000
            console.log('votes2===', Number(votes2)); // 19963926623437730000
            console.log('votes3===', Number(votes3)); // 29945889697333154000

        } catch (error: any) {
            console.log('error===', error);
        }
    });

Tools Used

Manual Analysis

Recommended Mitigation Steps

check for existing delegatedId or keep track using mapping insted of array

add check for referrer and msg.sender can't be same

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

same address will intiate two different transactions.

add function to withdraw eth

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

here msg.value can be more than needed. Add function to windraw eth which can be extra deposited eth or accidently sent to this contract .

multiply before divide

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

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Unhandled return values of transfer for ERC20 transfer

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

Not checking return values of transfer for WETH/ERC20 could lead to loss of user's fund on transfer failure. ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures.

Unlocked pragma

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/GolomToken.sol#L2

GolomToken has pragma solidity version number with ^0.8.11. The caret (^) points to unlocked pragma, meaning compiler will use the specified version or above. It's good practice to use specific solidity version to know compiler bug fixes and optimisations were enabled at the time of compiling the contract.

Zero address validation

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

add zero address check validation

remove unnecessary code

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

remove unnecessary commented code

add clear error message

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

it could be Insufficient ETH