Golom contest - jayjonah8's results

Lines of code

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

Vulnerability details

Impact

In GolomTrader.sol the 'payEther()' function handles royalty transfers. It uses '.transfer()' to send ether which has a 2300 gas limit. In running the test's included in the repo, it clearly shows that every function that uses 'payEther()' uses far more than the 2300 gas limit even up until the first time that 'payEther()' is called which means every function that uses 'payEther()' can fail. All the main functions in GolomTrader.sol use this function including 'fillAsk()', and '_settleBalances()' which is called at the end of 'fillCriteriaBid()' and 'fillBid()'. This impacts the core functionality of the protocol.

Proof of Concept

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

https://solidity-by-example.org/sending-ether/

Tools Used

Manual code review

Recommended Mitigation Steps

It's recommended to use '.call() ' when sending ether instead which forwards all gas and returns a boolean and to guard against reentrancy with reentrancy guards.

Lines of code

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

Vulnerability details

Impact

In GolomTrader.sol the 'validateOrder()' function makes a call to 'ecrecover()' to grab the 'signaturesigner'. The problem is that if 'ecrecover()' fails it will return a 0 which is currently not guarded against. This was a problem in the famous Polygon Matic token bug. There should be validation that the return value from 'ecrecover()' is not zero to prevent malicious hashes from being passed in. Also The next 4 lines of code present a problem which will never allow the if statement to run even when it should.

    address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
    require(signaturesigner == o.signer, 'invalid signature');
    if (signaturesigner != o.signer) {
        return (0, hashStruct, 0);
    }

With the current require statement here. The following if statement could never be true. This means the 'validateOrder()' function could never return an OrderStatus of 0 which should be an option for the function to return.

Proof of Concept

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

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

Tools Used

Manual code review

Recommended Mitigation Steps

Add: address signaturesigner = ecrecover(hash, o.v, o.r, o.s); require(signaturesidnger != 0, "Recovery failed")

Also the 'require(signaturesigner == o.signer, 'invalid signature');' should be removed and everywhere 'validateOrder()' is called the Order status should be validated by reverting if it is 0.