AccountingManager is not ERC4626 compliant

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/accountingManager/AccountingManager.sol#L693-L707
https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/accountingManager/AccountingManager.sol#L304
https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/accountingManager/AccountingManager.sol#L200

Vulnerability details

As stated in the README, the AccountingManager should comply with the ERC4626 standard, but it currently does not.

Impact

Vault does not conform to ERC4626 which may break external integrations.

Proof of Concept

Functions:

1. Deposit and Mint Functions:
   • ERC-4626 Functions:
     • deposit(uint256 assets, address receiver)
     • mint(uint256 shares, address receiver)
   • AccountingManager:
     • The AccountingManager contract overrides these functions but reverts with NoyaAccounting_NOT_ALLOWED.
     • The contract has its custom deposit function, but it takes three parameters (address receiver, uint256 amount, address referrer) instead of the ERC-4626 standard two parameters (uint256 assets, address receiver).
2. Withdraw and Redeem Functions:
   • ERC-4626 Functions:
     • withdraw(uint256 assets, address receiver, address owner)
     • redeem(uint256 shares, address receiver, address owner)
   • AccountingManager:
     • Similar to the deposit and mint functions, the AccountingManager overrides these functions but reverts with NoyaAccounting_NOT_ALLOWED.
     • The contract has its custom withdraw function, but it only takes two parameters (uint256 share, address receiver), whereas the ERC-4626 standard requires three parameters (uint256 assets, address receiver, address owner).
     • The AccountingManager.withdraw function is actually aERC4626.redeem function. The reason for this is it receives uint256 share as a parameter when it should instead receive uint256 assets.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the AccountingManager functions to comply with the ERC4626 standard.

Assessed type

ERC4626

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #136

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by gzeon-c4

[c4-judge]

gzeon-c4 marked the issue as partial-25

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)