All Tokens Can Be Stolen From Shelter Contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

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);
    }

In the above withdraw function in the Shelter contract, the claimed mapping is never checked to see if the msg.sender has already claimed their tokens already. As a result, the user can keep on claiming and claiming until all of the tokens are stolen. Furthermore, the _to parameter is used in the claimed[_token][_to] = true; definition. This allows the attacker to claim their own token amount under another EOA's address. An attacker with money locked in the shelter can generate an infinite amount of EOAs and call the withdraw function with those addresses in the _to field from the attacker's address and steal all the tokens since the _to is changing every time and the msg.sender is kept consistent.

Tools Used

Manual analysis

Recommended Mitigation Steps

I'd recommend removing the _to variable and using msg.sender instead. Additionally, the claimed mapping should be checked to see if the user has already claimed their tokens.

[GalloDaSballo]

Dup of #246

[GalloDaSballo]

Dup of #103