Badger Citadel contest - p4st13r4's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L315

Vulnerability details

Impact

The setSaleRecipient() function can change the sale recipient while a sale is open. This function can be only called by the owner of the contract, however, since:

1. the saleRecipient is the address that receives all the input tokens deposited by users, and

2. a comment explicitly says that this address will be citadel multisig (https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L92),

this param should not be something that can be changed easily, especially not while a sale is open. In fact this opens up scenarios of funds theft by malicious contract owners

Proof of Concept

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L315

Tools Used

Editor

Recommended Mitigation Steps

Add this check:

require(!finalized, "TokenSale: already finalized");