Golom contest - 8olidity's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L375-L403 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/DummyRewardDistributor.sol#L39-L41 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L141-L152 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L155-L166 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L172-L210

Vulnerability details

Impact

The return value of an external transfer/transferFrom call is not checked

Proof of Concept

Several tokens do not revert in case of failure and return false. If one of these tokens is used in MyBank, deposit will not revert if the transfer fails, and an attacker can call deposit for free..

Tools Used

vscode

Recommended Mitigation Steps

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

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

Proof of Concept

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) tha

Tools Used

vscode

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. Note that the CallyNft contract should inherit the ERC721TokenReceiver contract as a consequence.

safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

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

Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file

File: https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L181
        //deadline
        if (block.timestamp > o.deadline) {
            return (1, hashStruct, 0);
        }

Legacy debug information

File: contracts\rewards\RewardDistributor.sol

L9:  
import 'hardhat/console.sol';

L98:
function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
    //console.log(block.timestamp,epoch,fee);

File: contracts\vote-escrow\VoteEscrowDelegation.sol:
  217  
  218:     // /// @notice Remove delegation by user
  219      // function removeDelegationByOwner(uint256 delegatedTokenId, uint256 ownerTokenId) external {

_airdrop maybe address(0)

Malicious admin may give 0 address mint token

poc

File: contracts\governance\GolomToken.sol:
  41      /// @param _airdrop Airdrop contract
  42:     function mintAirdrop(address _airdrop) external onlyOwner {
  43          require(!isAirdropMinted, 'already minted');

Missing checks for address(0x0) when assigning values to _token state variables

File: contracts\vote-escrow\VoteEscrowDelegation.sol:
  52      constructor(address _token) {
  53:         token = _token;
  54          voter = msg.sender;

MISSING EVENT FOR CRITICAL PARAMETER CHANGE

File: contracts\vote-escrow\VoteEscrowDelegation.sol:
  259      /// @param _newMinVotingPower New minimum voting power required
  260:     function changeMinVotingPower(uint256 _newMinVotingPower) external onlyOwner {
  261          MIN_VOTING_POWER_REQUIRED = _newMinVotingPower;