ERC20 tokens can be incorrectly burnt because of insufficient validation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L24-L32 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L16-L19 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L33-L42

Vulnerability details

Impact

ERC20 tokens are incorrectly burnt.

Proof of Concept

In the file WildcatSanctionsEscrow.sol there is a constructor function:

  constructor() {
    sentinel = msg.sender;
    (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();
  }

Let's suppose this constructor function is called. In such a case sentinel is set to msg.sender and the line:

   (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();

is executed. Let's now suppose on the address of sentinel an instance of WildcatSanctionsSentinel has been deployed - this means that the constructor function from the file WildcatSanctionsSentinel.sol has been run:

  constructor(address _archController, address _chainalysisSanctionsList) {
    archController = _archController;
    chainalysisSanctionsList = _chainalysisSanctionsList;
    _resetTmpEscrowParams();
  }

At the end of the constructor, the function _resetTmpEscrowParams:

  function _resetTmpEscrowParams() internal {
    tmpEscrowParams = TmpEscrowParams(address(1), address(1), address(1));
  }

is run and tmpEscrowParams is set to TmpEscrowParams(address(1), address(1), address(1));.

Let's suppose the function releaseEscrow from the file WildcatSanctionsEscrow.sol is called next:

  function releaseEscrow() public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount);

    emit EscrowReleased(account, asset, amount);
  }
}

and on the line:

IERC20(asset).transfer(account, amount);

the tokens are sent to account. However, before that, on the line:

(borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();

account has been set to address(1) and the transfer function sends the tokens to address(1), essentially burning them, which is not the desired behavior.

Tools Used

Manual review.

Recommended Mitigation Steps

The function releaseEscrow should ensure that account != address(1). That can be achieved with adding a require statement:

  function releaseEscrow() public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();
    require(account != address(1), "Should not send tokens to reset account");

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount);

    emit EscrowReleased(account, asset, amount);
  }
}

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Invalid

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Invalid