Golom contest - apostle0x01's results

Lines of code

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

Vulnerability details

Details & Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. We can state that this isn’t recommended because:

• OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
• Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()
• Past audits have seen same issue being exploited such as in : https://github.com/code-423n4/2022-05-cally-findings/issues/38

POC

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

ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

[L-01] Unused receive() function will lock Ether in contract

Impact

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

governance/Timlock.sol:154:    receive() external payable {}
core/GolomTrader.sol:461:    receive() external payable {}
rewards/RewardDistributor.sol:315:    receive() external payable {}

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/Timlock.sol#L154 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L461 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L315

https://github.com/Quillhash/QuillAudit_Reports/blob/master/HarmonyLauncher%20Smart%20Contract%20Audit%20Report%20-%20QuillAudits.pdf

[L-02]. No error messages in require() functions

There are few places in the entire codebase where the require() statement does not contain any error message. As error messages are intended to notify users about failing conditions, they should provide enough information so that appropriate corrections can be made to interact with the system. Below is a non-exhaustive list of identified instances:

vote-escrow/VoteEscrowCore.sol:889:        require(msg.sender == voter);
vote-escrow/VoteEscrowCore.sol:895:        require(_from != _to);
vote-escrow/VoteEscrowCore.sol:896:        require(_isApprovedOrOwner(msg.sender, _from));
vote-escrow/VoteEscrowCore.sol:897:        require(_isApprovedOrOwner(msg.sender, _to));
vote-escrow/VoteEscrowCore.sol:944:        require(_value > 0); // dev: need non-zero value
vote-escrow/VoteEscrowDelegation.sol:245:  require(_isApprovedOrOwner(_sender, _tokenId));
rewards/RewardDistributor.sol:158:            require(epochs[index] < epoch);
core/GolomTrader.sol:350:        require(amountRemaining >= amount);
core/GolomTrader.sol:342:        require(o.totalAmt >= o.exchange.paymentAmt + o.prePayment.paymentAmt + o.refererrAmt);

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L889 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L895 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L896 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L897 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L944 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L245 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L158 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L350 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L342

Remediation

Lack of error messages greatly damage the overall user experience, thus lowering the system's quality. Consider not only fixing the specific instances mentioned above, but also reviewing the entire codebase to make sure every error message is informative and user-friendly.

[G-01] Splitting require() statements that use && saves gas

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

POC

Vulnerable location:

vote-escrow/VoteEscrowCore.sol:1008:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
vote-escrow/VoteEscrowCore.sol:538:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
vote-escrow/VoteEscrowCore.sol:894:        require(attachments[_from] == 0 && !voted[_from], 'attached');
vote-escrow/VoteEscrowDelegation.sol:239:        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');
	

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L239 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L538 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L894 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1008

MITIGATION

Breakdown each condition in a separate require

require(attachments[_tokenId] == 0);
require(!voted[_tokenId], 'attached');

[G-02] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowCore.sol:745:            for (uint256 i = 0; i < 255; ++i) {
vote-escrow/VoteEscrowCore.sol:1044:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1115:        for (uint256 i = 0; i < 128; ++i) {
vote-escrow/VoteEscrowCore.sol:1167:        for (uint256 i = 0; i < 255; ++i) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L745 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1044 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1115 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1167 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L415 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L183 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L226 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L258 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L273

Mitigation

As an example: for (uint256 index = 0; i < length;) { should be replaced with for (uint256 index; index < length;) {

[G-03] Cache storage values in memory

Impact

Anytime you are reading from storage more than once, it is cheaper to cache variables in memory. An SLOAD cost 100 GAS while MLOAD and MSTORE only cost 3 GAS.

This is especially true in for loops when using the length of a storage array as the condition being checked after each loop. Caching the array length in memory can yield significant gas savings when the array length is high.

// Before
for(uint 1; i < approvals.length; i++); // approvals is a storage variable

// After
uint memory numApprovals = approvals.length;
for(uint 1; i < numApprovals; i++);

POC

vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L171 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L189 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L199 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L415 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L143 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L157 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L180 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L183

[G-04] Increments can be unchecked

Impact

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Instances include:

vote-escrow/VoteEscrowDelegation.sol:171:        for (uint256 index = 0; index < delegated.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:189:        for (uint256 index = 0; index < delegatednft.length; index++) {
vote-escrow/VoteEscrowDelegation.sol:199:        for (uint256 i; i < _array.length; i++) {
core/GolomTrader.sol:415:        for (uint256 i = 0; i < proof.length; i++) {
rewards/RewardDistributor.sol:143:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:157:        for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:180:        for (uint256 tindex = 0; tindex < tokenids.length; tindex++) {
rewards/RewardDistributor.sol:183:            for (uint256 index = 0; index < epochs.length; index++) {
rewards/RewardDistributor.sol:226:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:258:        for (uint256 index = 0; index < epoch; index++) {
rewards/RewardDistributor.sol:273:        for (uint256 index = 0; index < epoch; index++) {

The code would go from:

for (uint256 index = 0; index < delegated.length; index++) {  
 // ...  
}  

to:

for (uint256 index; index < delegated.length;) {  
 // ...  
 unchecked { ++index; }  
}  

The risk of overflow is inexistant for a uint256 here.