Popcorn contest - pavankv's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L153

Vulnerability details

Impact

A transfer-on-fee token or a deflationary/rebasing token, causing the received amount to be less than the accounted amount. For instance, a deflationary tokens might charge a certain fee for every transfer()/transferFrom() or safeTransfer()/safeTransferFrom() .

Summary

User calls _deposit(address,address,100,100) if assets is deflationary tokens or transfer-on-fee tokens ,AdapterBase.sol receives only 99 due to transfer-on-fee tokens .And in line AdapterBase.sol#L156 it takes direct assets from parameter as 100 but in reality AdapterBase.sol receives only 99 , protocolDeposit() consider 100 tokens only.

Proof of Concept

Below mentioned line also have same issue https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardEscrow.sol#L100 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L153 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L230 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol#L170 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L153 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L193 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol#L457

Tools Used

Manual View

Recommended Mitigation Steps

To mitigate this issue maintain before and after balance

change to :-

uint balBefore = balanceOf(address(this)); IERC20(asset()).safeTransferFrom(caller, address(this), assets); uint balAfter = balanceOf(address(this)); uint received = balAfter-balBefore; _protocolDeposit(received, shares);

Here _protocolDeposit() get exact amount tokens received .

Reference:-

https://github.com/code-423n4/2022-12-prepo-findings/issues/13

1 . Include return parameters in NatSpec comments

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

If Return parameters are declared, you must prefix them with ”/// @return”.

Some code analysis programs do analysis by reading NatSpec details, if they can’t see the “@return” tag, they do incomplete analysis. Recommended Mitigation Steps Include return parameters in NatSpec comments

codesnippet:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L89 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L110 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L129 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L147 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L173 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L193 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol#L203 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol#L242

2 .Add Zero check for shares :-

code snippet:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L129 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L118 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L129

3 . modifiers should be use only for external or public not internal functions :-

modifiers should be use only for external or public not internal functions because internal function can call by internal only so add to deposit() in line AdapterBase.sol#L110. Internals cannot called by external users so add to exteranl function only to mint() , deposit() and other .

code snippet:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L152

4 .Events should emit after operation :-

In below mentioned code emit should after operation because if adapter didn't approve but event emitted changed adapter .

code snippeT:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L606

5.Remove empty blocks to save gas:-

Remove empty blocks because while deploying even single letter will consume the gas .

code snippet:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L477

6 . No events for changes :-

In changeRewardSpeed() changes the reward speed but no events to emit ,it a good practice to add event to changes.

code snippet:- https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol#L296