Holograph contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/enforcer/HolographERC721.sol#L392

Vulnerability details

Impact

beforeApprovalAll() / afterApprovalAll() can only pass "to" and "approved", missing "owner", if contract listening to this event,but does not know who approve it, so can not react to this event Basically, this event cannot be used

Proof of Concept

  function setApprovalForAll(address to, bool approved) external {
....

    if (_isEventRegistered(HolographERC721Event.beforeApprovalAll)) {
      require(SourceERC721().beforeApprovalAll(to, approved)); /***** only to/approved ,need owner
    }  

    _operatorApprovals[msg.sender][to] = approved;

    if (_isEventRegistered(HolographERC721Event.afterApprovalAll)) {
      require(SourceERC721().afterApprovalAll(to, approved)); /***** only to/approved ,need owner
    }
  }

Tools Used

Recommended Mitigation Steps

add parameter: owner

interface HolographedERC721 {
...

- function beforeApprovalAll(address _to, bool _approved) external returns (bool success);
+ function beforeApprovalAll(address owner, address _to, bool _approved) external returns (bool success);

- function afterApprovalAll(address _to, bool _approved) external returns (bool success);
+ function afterApprovalAll(address owner, address _to, bool _approved) external returns (bool success);

  function setApprovalForAll(address to, bool approved) external {

    if (_isEventRegistered(HolographERC721Event.beforeApprovalAll)) {
-     require(SourceERC721().beforeApprovalAll(to, approved)); 
+     require(SourceERC721().beforeApprovalAll(msg.sender,to, approved)); 
    }  

    _operatorApprovals[msg.sender][to] = approved;

    if (_isEventRegistered(HolographERC721Event.afterApprovalAll)) {
-      require(SourceERC721().afterApprovalAll(to, approved));
+      require(SourceERC721().afterApprovalAll(msg.sender,to, approved));
    }
  }

1: UNSAFE USAGE OF ERC20 TRANSFER

Some ERC20 tokens functions don’t return a boolean, for example USDT, BNB, OMG. So the Multiswap contract simply won’t work with tokens like that as the token.

The USDT’s transfer and transferFrom functions doesn’t return a bool, so the call to these functions will revert although the user has enough balance and the PA1D contract won’t work, assuming that token is USDT.

  function _payoutToken(address tokenAddress) private {
...
    for (uint256 i = 0; i < length; i++) {

      require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token"); /**** UNSAFE USAGE OF ERC20 TRANSFER **/

    }
  }

Suggestions: Use the OpenZepplin’s safeTransfer and safeTransferFrom functions.

L2: configurePayouts() Add the judgment that payaddress cannot be address(0); PA1D.sol

  function configurePayouts(address payable[] memory addresses, uint256[] memory bps) public onlyOwner {

    for (uint256 i = 0; i < addresses.length; i++) {
+     require(addresses[i]!=address(0));
      totalBp = totalBp + bps[i];
    }
...
  }

L-03: Variable naming error

- function getDeploymentBlock() external view returns (address holograph) {
+ function getDeploymentBlock() external view returns (address blockHeight) {
    assembly {
-     holograph := sload(_blockHeightSlot)
+     blockHeight := sload(_blockHeightSlot)
    }
  }