code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L52-L58

Vulnerability details

Impact

The claimed mapping is checked on the _to address, hence malicious users could frontrun calls to withdraw and potentially prevent successful withdrawals if the claimed mapping is correctly checked.

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L52-L58

function withdraw(IERC20 _token, address _to) external override {
  require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
  uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
  claimed[_token][_to] = true;
  emit ExitShelter(_token, msg.sender, _to, amount);
  _token.safeTransfer(_to, amount);
}

Tools Used

Manual code review. Confirmation from Taek.

Recommended Mitigation Steps

Consider replacing _to with msg.sender.

r2moon commented 2 years ago

https://github.com/code-423n4/2022-02-concur-findings/issues/246

GalloDaSballo commented 2 years ago

Disagree that this is a duplicate of 246.

This is an additional vulnerability related to the same code.

Here the warden shows how you could spin up a new account to claim 0 tokens and grief the withdrawal of someone who is entitled to some tokens (technically they just need to send to another address)

Given these considerations, I believe Low Severity to be more appropriate (DOS that can be easily sidestepped)

JeeberC4 commented 2 years ago

Generating QA Report as warden had not submitted one and judge downgraded issue. Preserving original title: Users Can Deny Other Users From Calling Shelter.withdraw()

code-423n4 / 2022-02-concur-findings