Concur Finance contest - Czar102's results

Lines of code

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

Vulnerability details

Impact

Shelter has a function withdraw that lets whitelisted users withdraw a specified amount of some token. The function does not check if the user has already withdrew the tokens. Since, a user can withdraw allowed amount any number of times, stealing all the funds.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a check:

require(claimed[_token][msg.sender] == false, "already withdrew");

And change current:

claimed[_token][_to] = true;

to

claimed[_token][msg.sender] = true;

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L86-L101

Vulnerability details

Impact

Token fee in MasterChef can be set to more than 100%, (for example by accident) causing all deposit calls to fail due to underflow on subtraction when reward is lowered by the fee, thus breaking essential mechanics. Note that after the fee has been set to any value, it cannot be undone. A token cannot be removed, added, or added the second time. Thus, mistakenly (or deliberately, maliciously) added fee that is larger than 100% will make the contract impossible to recover from not being able to use the token.

Tools Used

Manual analysis

Recommended Mitigation Steps

On setting fee ensure that it is below a set maximum, which is set to no more than 100%.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L78-L80 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L157-L180

Vulnerability details

Impact

Owner can steal Concur rewards by adding a depositor and inflating other depositors' assigned balance of the token within the contract. Thus, the owner-managed depositor can get most (all but one wei) of the created tokens.

Tools Used

Manual analysis

Recommended Mitigation Steps

Do not allow the owner to add depositors after the depositors have been configured.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L82-L84

Vulnerability details

Impact

Owner can remove a depositor. Since only depositors can deposit and withdraw, the owner may add a contract to the whitelist, let users deposit in the contarct and remove the depositor from the whitelist. Depositor's reward cannot be withdrawn then. And takes a share of Concur tokens that will not be ditributed.

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove onlyDepositor modifier from the withdraw function.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L134-L154

Vulnerability details

Impact

The updatePool function in MasterChef is supposed to account for all blocks before endBlock. But the if statement checks if the current block is after the deadline. Thus blocks between pool.lastRewardBlock and endBlock won't be accounted for iff block.number >= endBlock && endBlock > pool.lastRewardBlock.

Additionally, the pendingConcur function does not have the endBlock functionality at all and this will lead to incorrect information given after the deadline.

Tools Used

Manual analysis

Recommended Mitigation Steps

Check if the above condition holds, if yes, account for all blocks between pool.lastRewardBlock and endBlock.

To avoid dissociations of the values in the code, it is recommended to create a single function for such critical calculations and call it anytime the calculations are needed.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L34-L41

Vulnerability details

Impact

Any address that has nonzero reward for a token _tokens[i] is able to drain all contact token funds if the transfer function is reentrant (for example, ERC777 token). As _tokens[i] is an arbitrarily implemented, a reentrant transfer function can be assumed to be present among all _tokens.

Proof of Concept

Let there be an address addr that is a contarct and has its reward pushed on token token that has a hookable transfer function.

An exploiter created the addr contract in a way that it hooked the transfer function if tokens are sent to it from ConcurRewardPool. Contract can be called to claimRewards from ConcurRewardPool with a list of length 1, consisting of token. When a transfer occurs, the hook passes control back to the addr, which can call claimRewards again with the same argument and payout the same amount as reward[msg.sender][_tokens[i]] asn't been yet modified.

Tools Used

Manual analysis

Recommended Mitigation Steps

Swap lines 37 and 38 in the claimRewards implementation.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L201-L211

Vulnerability details

Impact

All calculations are rounded down, since a lack of tokens in the contracts cannot be rounding errors' fault. So the function is redundant.

On the other hand, if the contract is undersupplied with Concur tokens, this will cause depositors to be sent less tokens than needed (or none). This is especially unsafe because the tokens that were lacking are not resembled in accountings at all. Thus a depositor may invoke the safeConcurTransfer and not receive tokens they were supposed to.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use usual safeTransfer instead of safeConcurTransfer.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L50

Vulnerability details

Impact

Block average time can change significantly, changing protocol parameters. If time dependence is desired, use block.timestamp. This can vary by as much as 15s (default Geth implementation), while block times can change measured time not by a constant, but scale it. Note that block time change WILL happen because of difficulty bomb and Ethereum 2.0.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use block.timestamp for calculations where time is crucial.