Golom contest - M0ndoHEHE's results

Lines of code

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

Vulnerability details

Impact

ETH sent to the contract are wrapped within the addFee function from RewardDistributor contract. When rewardToken.totalSupply is greater than 1000000000 the function just returns. This will stop wrapping the ETH used to pay rewards using WETH.

This vulnerability will impact these functions within the RewardDistributor contract:

• addFee
• multiStakerClaim

Proof of Concept

the addFee code is called from GolomTrader contract within fillAsk and _settleBalance.

ref1:

        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
            // if supply is greater then a billion dont mint anything, dont add trades
            return;
        }

so whenever the total supply is bigger than 1000000000, the function just returns, this would skip these lines that are used to wrap ETH sent to the contract: ref2

            if (previousEpochFee > 0) {
                if (epoch == 1){
                    epochTotalFee[0] =  address(this).balance; // staking and trading rewards start at epoch 1, for epoch 0 all contract ETH balance is converted to staker rewards rewards.
                    weth.deposit{value: address(this).balance}();  
                }else{
                    weth.deposit{value: previousEpochFee}();
                }
            }

in this way all the ETH sent to the contract will be lost forever!

Another unexpected behaviour caused by this bug is within multiStackerClaim function: ref3

        weth.transfer(tokenowner, rewardEth);
    }

This last line will never send weth to tokenowner.

Tools Used

Manual Audit

Recommended Mitigation Steps

Keep calling weth.deposit() even if the total supply of reward tokens have reach the ceiling.

Lines of code

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

Vulnerability details

Impact

ERC721 standard requires the receiving contract to implement onERC721Received(), this may lead to loss of ERC721 if it doesn't implement it.

Proof of Concept

ref

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
        } else {

ref

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
        } else {

ref

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, tokenId);
        } else {

Tools Used

Manual Audit

Recommended Mitigation Steps

To solve this issue use the safeTransferFrom() function, that checks whether the receiving contract is IERC721Receiver.