Golom contest - TrungOre's results

Lines of code

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

Vulnerability details

Impact

• Denial of Service because ve always is address(0)

Proof of concept

There is no initial setup for ve variable in contract RewardDistributor. So if we want to set ve variable, we will need to call function addVoteEscrow().

function addVoteEscrow(address _voteEscrow) external onlyOwner {
    if (address(ve) == address(0)) {
        ve = VE(pendingVoteEscrow);
    } else {
        voteEscrowEnableDate = block.timestamp + 1 days;
        pendingVoteEscrow = _voteEscrow;
    }
}

Function addVoteEscrow() will check address(ve) == address(0) or not. Cause there is no initialization for ve, it will set ve = VE(pendingVoteEscrow). But at this time, pendingVoteEscrow is address(0) too.
==> ve == address(0) forever

Tools Used

Manual review

Recommended Mitigation Steps

change function to

function addVoteEscrow(address _voteEscrow) external onlyOwner {
    if (address(ve) == address(0)) {
        ve = VE(_voteEscrow);
    } else {
        voteEscrowEnableDate = block.timestamp + 1 days;
        pendingVoteEscrow = _voteEscrow;
    }
}

Lines of code

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

Vulnerability details

Impact

User can't transfer/transferFrom NFT

Proof of concept

Function transfer() and transferFrom() will execute internal function _transferFrom() in contract VoteEscrowDelegation.sol. Function _transferFrom() use command

this.removeDelegation(_tokenId);

to remove the delegation. But use this. will let the function execute a external call to itself. That will make the requirement

require(ownerOf(tokenId) == msg.sender, 'VEDelegation: Not allowed');

failed, cause the current msg.sender now is its address (not the origin sender and not the owner of nft).

Tools Used

Manual review

Recommended Mitigation Steps

delete this. to make a internal call.

// this.removeDelegation(_tokenId);
removeDelegation(_tokenId);

Lines of code

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

Vulnerability details

Impact

To withdraw eth it uses transfer(), this trnansaction will fail inevitably when : -

1. The withdrawer smart contract does not implement a payable function.

2. Withdrawer smart contract does implement a payable fallback which uses more than 2300 gas unit

3. The withdrawer smart contract implements a payable fallback function whicn needs less than 2300 gas unit but is called through proxy that raise the call's gas usage above 2300

Proof of concept

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

Tools Used

Manual review

Recommended Mitigation Steps

use call() to send eth

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L301 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

Vulnerability details

Impact

In the function fillBid() of contract PrizePool, when transfering ERC721 tokens to the msg.sender, the transferFrom keyword is used instead of safeTransferFrom. If any msg.sender is a contract and not aware of incoming ERC721 tokens, the sent tokens could be locked.

Proof of concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider changing transferFrom to safeTransferFrom

[2022-07-golom] QA report

tags: c4, 2022-07-golom, QA

should revert a mean message

change 'mgmtm' to another message, example 'not enough value' https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol/#L217

inconsistence between bid and ask

function fillBid and fillCriteriaBid: sender should send msg.value to p.paymentAddress instead of signer

should change requirement

• Change requirement to

uint256 protocolFee = ((o.totalAmt * 50) / 10000) * amount;
require(
    o.totalAmt * amount >=
        (o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt) * amount + p.paymentAmt + protocolfee
); // cause bidder eth is paying for seller payment p , dont take anything extra from seller

Because o.totalAmt should include protocolFee

• Change requirement from fillCriteriaBid similarly

should add error message

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L285-L296
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L342-L350
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L88

should remove unused library

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L9

should change interface

• should change weth to WETH9 cause ERC20 has no function called deposit() https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L69

typo comment

• epoc -> epoch https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L61-L64
• exeute -> execute https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L227

delete unnecessary comment console.log

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L99

use rewardToken.decimals() instead of hard coding 18

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L100

name of variables should use mixCase naming style

protocolfee -> protocolFee https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381-L384

remove unnecessary bracket

(epoch) -> epoch https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L106

ERC20: use safeTransfer instead of transfer

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L151
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L165
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L208-L209

shouldn't use uppercase for not-constant variable

MIN_VOTING_POWER_REQUIRED -> minVotingPowerRequired https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowDelegation.sol#L50