Golom contest - Twpony's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L85 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L101

Vulnerability details

Impact

When the user has owned tokenID, wants to call delegate(). At first numCheckpoints[toTokenId] is 0, so nCheckpoints = 0, but when _writeCheckpoint() calls checkpoints[toTokenId][nCheckpoints - 1], the underflow error will throw.

So the whole delegate() cannot start to run.

Proof of Concept

function delegate(uint256 tokenId, uint256 toTokenId) external {
        uint256 nCheckpoints = numCheckpoints[toTokenId];
        if (nCheckpoints > 0) {
        } else {  // nCheckpoints == 0
            uint256[] memory array = new uint256[](1);
            array[0] = tokenId;
            _writeCheckpoint(toTokenId, nCheckpoints, array);
        }
}
function _writeCheckpoint(
        uint256 toTokenId,
        uint256 nCheckpoints,
        uint256[] memory _delegatedTokenIds
    ) internal {
        Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
}    

checkpoints[toTokenId][nCheckpoints - 1] will underflow, if nCheckpoints == 0.

When _writeCheckpoint is executed for tokenId for the first time, nCheckpoints will be 0. so delegate() function can not start to work.

Tools Used

Manual review

Recommended Mitigation Steps

Change Checkpoint memory oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1]; to like the following:

        Checkpoint memory oldCheckpoint;
        if (nCheckpoints == 0) {
            oldCheckpoint = checkpoints[toTokenId][0];
        } else {
            oldCheckpoint = checkpoints[toTokenId][nCheckpoints - 1];
        }

Lines of code

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

Vulnerability details

Impact

delegate() does not set useful limits for tokenId delegation. For example, tokenId can not be equal to toTokenId, otherwise every tokenId can delegate to itself to get votes; after delegation, there is no state change for tokenId, it has the same capability to go on delegating.

function delegate(uint256 tokenId, uint256 toTokenId) external {}

``

Proof of Concept

The following is the test example for delegate(). The result shows if you have two tokenId, you can delegate from one tokenId to the other tokenId, the votes of delegated tokenId will increase to any value you want.

pragma solidity >=0.6.0 <0.9.0;
import "forge-std/Test.sol";
import {VoteEscrow} from "../src/golom/vote-escrow/VoteEscrowDelegation.sol";
import {ERC20Mock} from "../src/golom/test/ERC20Mock.sol";

contract VoteEscrowTest is Test {
    VoteEscrow vescrow;
    ERC20Mock erc20mock;
    address bob = mkaddr("bob");
    uint256 lock_duation = 3600 * 24 * 7;
    uint256 minDeposit = 4 * 365 * 86400;

    function setUp() public {
        erc20mock = new ERC20Mock();
        vescrow = new VoteEscrow(address(erc20mock));
    }

    function testVoteEscrow() public {
        vm.warp(1641000000);

        vm.deal(bob, 100 ether);
        erc20mock.transfer(bob, 10000 ether);

        vm.startPrank(bob, bob);
        erc20mock.approve(address(vescrow), 10000 ether);
        vescrow.create_lock(10000, lock_duation);
        vescrow.create_lock(10000, lock_duation);
        vescrow.create_lock(10000, lock_duation);
        vescrow.create_lock(10000, lock_duation);

        vm.warp(1641100000);
        vescrow.deposit_for(1, minDeposit + 1);
        vm.stopPrank();
        
        vm.roll(3);
        vm.warp(1641150000);

        vm.startPrank(bob, bob);
        vescrow.delegate(1, 1);
        emit log_named_uint("GetVotes(1):", vescrow.getVotes(1));
        vescrow.delegate(1, 1);
        emit log_named_uint("GetVotes(1):", vescrow.getVotes(1));
        vescrow.delegate(1, 1);
        emit log_named_uint("GetVotes(1):", vescrow.getVotes(1));

        vescrow.delegate(1, 3);
        emit log_named_uint("GetVotes(3):", vescrow.getVotes(3));
        vescrow.delegate(1, 3);
        emit log_named_uint("GetVotes(3):", vescrow.getVotes(3));
        vescrow.delegate(1, 3);
        emit log_named_uint("GetVotes(3):", vescrow.getVotes(3));
        vm.stopPrank();
    }

    function mkaddr(string memory name) public returns (address) {
        address addr = address(
            uint160(uint256(keccak256(abi.encodePacked(name))))
        );
        vm.label(addr, name);
        return addr;
    }
}   

Tools Used

foundry

Recommended Mitigation Steps

Add the limits in delegate(), and set the state changeable variable for any tokenId to track the delegation of every tokenId, in order to avoid abuse of delegation.

mapping(uint256=>bool) public delegated;

function delegate(uint256 tokenId, uint256 toTokenId) external {
	require(tokenId != toTokenId, "tokenId can not delegate to itself");
	
	if (delegated[tokenId]==true){
		revert("token has been delegated");
	}
	....
	delegated[tokenId]=true;
}

Lines of code

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

Vulnerability details

Impact

fillAsk() in GolomTrader.sol, msg.sender call this function with msg.value. If every condition is satisfied, NFT will transfer to receiver, and msg.value will be distributed. But if the receiver is the contract and does not have code to deal with NFT, msg.sender will lose the ether, and NFT will be locked in receiver.

fillBid() and fillCriteriaBid() also use transferFrom for NFT transfer. It is suggested to modify like fillAsk(). But I think the risk of fillAsk() is quite greater than the risk of fillBid() and fillCriteriaBid() . Because the receiver in fillBid() and fillCriteriaBid() is o.signer, not any receiver. o.signer generally has knowledge of this order and may know how to handle NFT. But any receiver may not.

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

//fillBid() and fillCriteriaBid()
ERC721 nftcontract = ERC721(o.collection);
nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

Proof of Concept

transferFrom is not a safe function for ERC721, it will not check whether the NFT receiver will support ERC721 or not. If receiver is a contract address that does not support ERC721, the NFT can be frozen in the contract receiver.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

    function fillAsk(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p,
        address receiver
    ) public payable nonReentrant {
        ....    
        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
        } else {
            ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
        }
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add safeTransferFrom interface in interface ERC721, and change the ERC721 transfer function from transferFrom to safeTransferFrom.

interface ERC721 {
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;
}

function fillAsk(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p,
        address receiver
    ) public payable nonReentrant {
        ....    
        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId);
        } 
        ...
    }

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L461 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L217 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L459

Vulnerability details

Impact

GolomTrader.sol has realized receive() and fallback(), the contract can receive ether, but it does not realize transfer or withdraw function for Ether. If user sends ether to GolomTrader.sol carelessly or send more ether in fillAsk(), the user can not get the ether back.

Proof of Concept

There is no function to realize the transfer of ETHER from the GolomTrader contract to other address. payEther is an internal function, can not be called externally.

In fillAsk(), it requires msg.value is more than or equal to the NFT amount and fee. If the user transfers more to GolomTrader.sol, the user can not get the money back.

function fillAsk() public payable nonReentrant {
    require(msg.value >= o.totalAmt * amount + p.paymentAmt, "mgmtm");
}

fallback() external payable {}

receive() external payable {}

Tools Used

Manual review

Recommended Mitigation Steps

To realize a function to transfer Ether out, like the following transferEther function, only the governance can call the function.

function transferEther(uint256 payAmt, address payAddress) external OnlyGovernance {
   if (payAmt > 0) {
      payable(payAddress).transfer(payAmt); 
  }
}